fix(agentplugins): separate ChatGPT target lifecycle - #49
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe installer now supports separate Codex and ChatGPT targets. It loads portable and official OpenAI packages, validates MCP and app manifests, plans ChatGPT bindings, stages compatible projections, and supports State v3 migration. ChangesChatGPT agent-plugin support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
61896ae to
cdf9805
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
install/integrationctl/agentplugins/adapters/loader/openai_plugin.go (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
pathparameter to avoid shadowing thepathpackage.The file imports the standard-library
pathpackage at line 6. The parameter at line 25 shadows it for the whole function body. The code compiles because the function does not callpath.Cleanorpath.IsAbs, but any later use inside this function will not resolve to the package.♻️ Proposed rename
-func (loader Loader) loadOpenAIPluginManifest(path string) (domain.PluginManifest, openAIComponentPaths, []domain.Diagnostic, string, error) { - body, exists, err := readRegularFile(path) +func (loader Loader) loadOpenAIPluginManifest(manifestPath string) (domain.PluginManifest, openAIComponentPaths, []domain.Diagnostic, string, error) { + body, exists, err := readRegularFile(manifestPath)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/integrationctl/agentplugins/adapters/loader/openai_plugin.go` at line 25, Rename the path parameter in Loader.loadOpenAIPluginManifest to a non-conflicting name such as manifestPath, and update all references within the function accordingly so the imported path package remains accessible.install/integrationctl/agentplugins/adapters/statev2/store.go (1)
114-119: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider restricting
FormatIDto the known values.The relaxation is scoped correctly:
FormatIDAgentPluginsV1keeps the schema URI requirement, andFormatIDOpenAIPlugindoes not need one.The condition at line 117 is an equality test, so any other non-empty
FormatIDalso skips the schema URI check. Line 114 only requires the field to be non-empty. A corrupted or unrecognizedFormatIDtherefore validates with an emptySchemaURI.An explicit check against the two known constants would keep this guard closed.
♻️ Proposed change
if installation.Package.FormatID == "" || installation.Source.TreeDigest == "" || installation.Package.ManifestDigest == "" { return fmt.Errorf("%s standard package binding is incomplete", prefix) } - if installation.Package.FormatID == domain.FormatIDAgentPluginsV1 && installation.Package.SchemaURI == "" { - return fmt.Errorf("%s portable standard package binding has no schema URI", prefix) + switch installation.Package.FormatID { + case domain.FormatIDAgentPluginsV1: + if installation.Package.SchemaURI == "" { + return fmt.Errorf("%s portable standard package binding has no schema URI", prefix) + } + case domain.FormatIDOpenAIPlugin: + default: + return fmt.Errorf("%s standard package binding has unknown format_id %q", prefix, installation.Package.FormatID) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/integrationctl/agentplugins/adapters/statev2/store.go` around lines 114 - 119, Restrict FormatID validation in the installation binding validation block to the two supported constants, domain.FormatIDAgentPluginsV1 and domain.FormatIDOpenAIPlugin. Reject any other value, while preserving the existing requirement for SchemaURI when FormatIDAgentPluginsV1 is used.install/integrationctl/agentplugins/adapters/loader/loader.go (2)
167-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing generic
sortedRawKeyshelper.
sortedRawKeysat lines 149-156 is already generic over the map value type and has an identical body. Call it withapp.Bindingsand deletesortedAppBindingNames.♻️ Proposed fix
At line 96:
- AppBindings: sortedAppBindingNames(app.Bindings), + AppBindings: sortedRawKeys(app.Bindings),Then remove the helper:
-func sortedAppBindingNames(values map[string]domain.AppBinding) []string { - keys := make([]string, 0, len(values)) - for key := range values { - keys = append(keys, key) - } - sort.Strings(keys) - return keys -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/integrationctl/agentplugins/adapters/loader/loader.go` around lines 167 - 175, Replace the call sites using sortedAppBindingNames with the existing generic sortedRawKeys helper, passing app.Bindings directly. Then delete sortedAppBindingNames, preserving the current sorted key behavior.
44-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSet
mcpPathandmcpDeclaredinside the format branches.Line 44 sets
mcpPathto the portablemcp.json, and line 76 overwrites it with.mcp.jsonfor the official format. Line 48 setsmcpDeclaredto true, but the portable path never reads it, becauseloadMCPat line 74 takes nodeclaredargument.Assigning these values in each branch would make the format-specific contract explicit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/integrationctl/agentplugins/adapters/loader/loader.go` around lines 44 - 48, Move the mcpPath and mcpDeclared assignments out of their shared initialization near appDeclared and into the corresponding portable and official format branches. Set the portable branch to use mcp.json and its declared-state expected by loadMCP, and set the official branch to use .mcp.json with its own declared-state, ensuring loadMCP receives or uses the branch-specific values consistently.install/integrationctl/agentplugins/adapters/loader/app.go (2)
99-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one shared diagnostic constructor.
appDiagnostichere,openAIMCPDiagnosticatinstall/integrationctl/agentplugins/adapters/loader/mcp.golines 166-171, andmcpDiagnosticat lines 204-215 have the same body. They differ only inBoundaryandPath. A single helper that accepts boundary and path would remove the duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/integrationctl/agentplugins/adapters/loader/app.go` around lines 99 - 104, Consolidate the duplicated diagnostic construction used by appDiagnostic, openAIMCPDiagnostic, and mcpDiagnostic into one shared helper accepting the boundary and path as parameters. Update each existing constructor to delegate to that helper while preserving its current severity, code, message, boundary, and path values.
72-77: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStore the trimmed app id.
Line 73 validates
strings.TrimSpace(id)againstappIDPattern. Line 77 stores the untrimmedidin the binding. An entry such as{"id":" asdk_app_docs_123 "}passes validation and is stored with the surrounding spaces.Normalize once and use the normalized value.
♻️ Proposed fix
var id string if rawID, ok := entry["id"]; !ok || json.Unmarshal(rawID, &id) != nil || !appIDPattern.MatchString(strings.TrimSpace(id)) { diagnostics = append(diagnostics, appEntryDiagnostic(name, "app_id_invalid", "app id must reference a registered app, connector, or app template")) continue } - binding := domain.AppBinding{Alias: name, ID: id, Raw: append(json.RawMessage(nil), raw...)} + binding := domain.AppBinding{Alias: name, ID: strings.TrimSpace(id), Raw: append(json.RawMessage(nil), raw...)}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/integrationctl/agentplugins/adapters/loader/app.go` around lines 72 - 77, The app ID validation in the entry-loading logic trims whitespace but stores the original value. Normalize id once after unmarshalling, validate the trimmed value against appIDPattern, and use that normalized value for domain.AppBinding.ID while preserving the existing invalid-entry diagnostic behavior.install/integrationctl/agentplugins/adapters/loader/loader_test.go (1)
51-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the typed
Requiredfield.The fixture at line 44 sets
"required":true, but no assertion readsenvelope.App.Bindings["docs"].Required. The typed boolean decoding atinstall/integrationctl/agentplugins/adapters/loader/app.golines 79-90 is therefore not covered by this test.💚 Proposed assertion
- if !envelope.App.Present || !envelope.App.Enabled || envelope.App.Bindings["docs"].ID != "asdk_app_docs_123" { + if !envelope.App.Present || !envelope.App.Enabled || envelope.App.Bindings["docs"].ID != "asdk_app_docs_123" || !envelope.App.Bindings["docs"].Required { t.Fatalf("app component = %+v", envelope.App) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/integrationctl/agentplugins/adapters/loader/loader_test.go` around lines 51 - 56, Extend the loader test assertions for envelope.App.Bindings["docs"] to verify its typed Required field is true, matching the fixture’s "required": true value. Keep the existing presence, enabled, ID, and raw-manifest assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@install/integrationctl/agentplugins/adapters/loader/app.go`:
- Around line 19-29: The new loaders check !exists before handling
readRegularFile errors, causing unreadable files to be reported as absent. In
install/integrationctl/agentplugins/adapters/loader/app.go lines 19-29, update
the Load app flow to handle err != nil before !exists and emit
app_manifest_read_failed; apply the same ordering in
install/integrationctl/agentplugins/adapters/loader/mcp.go lines 101-114 so
failures emit mcp_read_failed instead of mcp_manifest_missing.
In `@install/integrationctl/agentplugins/adapters/loader/mcp.go`:
- Around line 126-137: In mcp.go, change the wrapped-document handling to decode
into a fresh local map[string]json.RawMessage and assign it to serverDocuments
only after successful validation, preventing the wrapper key from being
retained; in loader_test.go lines 90-92, assert that envelope.MCP.Servers
contains exactly one entry and envelope.MCP.InvalidServer is empty.
In `@install/integrationctl/agentplugins/domain/types.go`:
- Around line 143-144: Confirm whether ComponentInventory is reachable from any
persisted Installation state; if so, prevent AppPresent and AppBindings from
being serialized into persisted state to preserve compatibility with strict V2
decoders. In install/integrationctl/agentplugins/domain/types.go:143-144, make
the necessary serialization change only if the inventory is persisted. In
install/integrationctl/agentplugins/adapters/statev2/store_test.go:90-94,
populate app data before Save so the test exercises the contract, or remove the
forbidden-key assertions if ComponentInventory is confirmed unreachable.
---
Nitpick comments:
In `@install/integrationctl/agentplugins/adapters/loader/app.go`:
- Around line 99-104: Consolidate the duplicated diagnostic construction used by
appDiagnostic, openAIMCPDiagnostic, and mcpDiagnostic into one shared helper
accepting the boundary and path as parameters. Update each existing constructor
to delegate to that helper while preserving its current severity, code, message,
boundary, and path values.
- Around line 72-77: The app ID validation in the entry-loading logic trims
whitespace but stores the original value. Normalize id once after unmarshalling,
validate the trimmed value against appIDPattern, and use that normalized value
for domain.AppBinding.ID while preserving the existing invalid-entry diagnostic
behavior.
In `@install/integrationctl/agentplugins/adapters/loader/loader_test.go`:
- Around line 51-56: Extend the loader test assertions for
envelope.App.Bindings["docs"] to verify its typed Required field is true,
matching the fixture’s "required": true value. Keep the existing presence,
enabled, ID, and raw-manifest assertions unchanged.
In `@install/integrationctl/agentplugins/adapters/loader/loader.go`:
- Around line 167-175: Replace the call sites using sortedAppBindingNames with
the existing generic sortedRawKeys helper, passing app.Bindings directly. Then
delete sortedAppBindingNames, preserving the current sorted key behavior.
- Around line 44-48: Move the mcpPath and mcpDeclared assignments out of their
shared initialization near appDeclared and into the corresponding portable and
official format branches. Set the portable branch to use mcp.json and its
declared-state expected by loadMCP, and set the official branch to use .mcp.json
with its own declared-state, ensuring loadMCP receives or uses the
branch-specific values consistently.
In `@install/integrationctl/agentplugins/adapters/loader/openai_plugin.go`:
- Line 25: Rename the path parameter in Loader.loadOpenAIPluginManifest to a
non-conflicting name such as manifestPath, and update all references within the
function accordingly so the imported path package remains accessible.
In `@install/integrationctl/agentplugins/adapters/statev2/store.go`:
- Around line 114-119: Restrict FormatID validation in the installation binding
validation block to the two supported constants, domain.FormatIDAgentPluginsV1
and domain.FormatIDOpenAIPlugin. Reject any other value, while preserving the
existing requirement for SchemaURI when FormatIDAgentPluginsV1 is used.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3502da60-a451-4e49-8f02-abfb1a400c74
📒 Files selected for processing (26)
README.mdcli/plugin-kit-ai/internal/agentpluginscli/add.gocli/plugin-kit-ai/internal/agentpluginscli/binding.gocli/plugin-kit-ai/internal/agentpluginscli/cli_test.gocli/plugin-kit-ai/internal/agentpluginscli/lifecycle.gocli/plugin-kit-ai/internal/agentpluginscli/read.gocli/plugin-kit-ai/internal/agentpluginscli/root.goinstall/integrationctl/agentplugins/adapters/clientdetect/detector.goinstall/integrationctl/agentplugins/adapters/clientdetect/detector_test.goinstall/integrationctl/agentplugins/adapters/loader/app.goinstall/integrationctl/agentplugins/adapters/loader/loader.goinstall/integrationctl/agentplugins/adapters/loader/loader_test.goinstall/integrationctl/agentplugins/adapters/loader/mcp.goinstall/integrationctl/agentplugins/adapters/loader/openai_plugin.goinstall/integrationctl/agentplugins/adapters/statemigration/migrate_test.goinstall/integrationctl/agentplugins/adapters/statev2/store.goinstall/integrationctl/agentplugins/adapters/statev2/store_test.goinstall/integrationctl/agentplugins/domain/clients.goinstall/integrationctl/agentplugins/domain/types.goinstall/integrationctl/agentplugins/planner/planner.goinstall/integrationctl/agentplugins/planner/planner_test.goinstall/integrationctl/agentplugins/providers/activator.goinstall/integrationctl/agentplugins/providers/activator_test.goinstall/integrationctl/agentplugins/providers/stager.goinstall/integrationctl/agentplugins/providers/stager_test.goinstall/integrationctl/agentplugins/usecase/service.go
| body, exists, err := readRegularFile(path) | ||
| component := domain.AppComponent{Present: exists, Declared: declared, Raw: append(json.RawMessage(nil), body...)} | ||
| if !exists { | ||
| if declared { | ||
| return component, []domain.Diagnostic{appDiagnostic("app_manifest_missing", "official manifest declares .app.json but the file is missing", nil)} | ||
| } | ||
| return component, nil | ||
| } | ||
| if err != nil { | ||
| return component, []domain.Diagnostic{appDiagnostic("app_manifest_read_failed", "read root .app.json", err)} | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Both new loaders discard read errors from readRegularFile. readRegularFile in install/integrationctl/agentplugins/adapters/loader/loader.go returns exists=false together with a non-nil error when os.Lstat fails for a reason other than "not exist". Both loaders evaluate the !exists branch first, so that error never reaches a diagnostic and an unreadable file is reported as absent. Load at line 38 of the same file already orders these checks correctly.
install/integrationctl/agentplugins/adapters/loader/app.go#L19-L29: move theerr != nilcheck above the!existscheck so a failed read emitsapp_manifest_read_failedinstead ofapp_manifest_missing.install/integrationctl/agentplugins/adapters/loader/mcp.go#L101-L114: move theerr != nilcheck above the!existscheck so a failed read emitsmcp_read_failedinstead ofmcp_manifest_missing.
📍 Affects 2 files
install/integrationctl/agentplugins/adapters/loader/app.go#L19-L29(this comment)install/integrationctl/agentplugins/adapters/loader/mcp.go#L101-L114
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install/integrationctl/agentplugins/adapters/loader/app.go` around lines 19 -
29, The new loaders check !exists before handling readRegularFile errors,
causing unreadable files to be reported as absent. In
install/integrationctl/agentplugins/adapters/loader/app.go lines 19-29, update
the Load app flow to handle err != nil before !exists and emit
app_manifest_read_failed; apply the same ordering in
install/integrationctl/agentplugins/adapters/loader/mcp.go lines 101-114 so
failures emit mcp_read_failed instead of mcp_manifest_missing.
| serverDocuments := rawFields | ||
| for _, wrapper := range []string{"mcp_servers", "mcpServers"} { | ||
| if wrapped, ok := rawFields[wrapper]; ok { | ||
| if len(rawFields) != 1 { | ||
| return component, []domain.Diagnostic{openAIMCPDiagnostic("mcp_servers_invalid", "wrapped .mcp.json cannot contain sibling fields", nil)} | ||
| } | ||
| if err := decodeJSON(wrapped, &serverDocuments); err != nil || serverDocuments == nil { | ||
| return component, []domain.Diagnostic{openAIMCPDiagnostic("mcp_servers_invalid", wrapper+" must be an object", err)} | ||
| } | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The wrapped .mcp.json form keeps the wrapper key as a server. serverDocuments aliases the non-nil rawFields map, and encoding/json merges into a non-nil map instead of replacing it. After unwrapping, the map holds the wrapper key and the real server entries, so the loader records a phantom mcp_server_invalid diagnostic named after the wrapper. The added test uses the wrapped form but asserts only the valid server, so it does not detect this.
install/integrationctl/agentplugins/adapters/loader/mcp.go#L126-L137: decode the wrapped value into a fresh localmap[string]json.RawMessageand assign it toserverDocumentsafter a successful decode.install/integrationctl/agentplugins/adapters/loader/loader_test.go#L90-L92: add assertions thatenvelope.MCP.Servershas exactly one entry andenvelope.MCP.InvalidServeris empty.
📍 Affects 2 files
install/integrationctl/agentplugins/adapters/loader/mcp.go#L126-L137(this comment)install/integrationctl/agentplugins/adapters/loader/loader_test.go#L90-L92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install/integrationctl/agentplugins/adapters/loader/mcp.go` around lines 126
- 137, In mcp.go, change the wrapped-document handling to decode into a fresh
local map[string]json.RawMessage and assign it to serverDocuments only after
successful validation, preventing the wrapper key from being retained; in
loader_test.go lines 90-92, assert that envelope.MCP.Servers contains exactly
one entry and envelope.MCP.InvalidServer is empty.
| AppPresent bool `json:"app_present,omitempty"` | ||
| AppBindings []string `json:"app_bindings,omitempty"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
The app inventory fields rely on an unverified serialization contract. The new AppPresent and AppBindings fields use omitempty, so they are omitted only while they are false or empty. The store test forbids those keys in written state, but its fixture has no app data, so the assertion passes without exercising the contract. Neither site establishes whether ComponentInventory is reachable from domain.Installation.
install/integrationctl/agentplugins/domain/types.go#L143-L144: confirm thatComponentInventoryis not embedded in any persisted state structure; if it is, a ChatGPT installation with app bindings will emit these keys and break old strict V2 decoders.install/integrationctl/agentplugins/adapters/statev2/store_test.go#L90-L94: populate app data on the installation beforeSave, or drop the two new forbidden keys once the reachability question is settled.
📍 Affects 2 files
install/integrationctl/agentplugins/domain/types.go#L143-L144(this comment)install/integrationctl/agentplugins/adapters/statev2/store_test.go#L90-L94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install/integrationctl/agentplugins/domain/types.go` around lines 143 - 144,
Confirm whether ComponentInventory is reachable from any persisted Installation
state; if so, prevent AppPresent and AppBindings from being serialized into
persisted state to preserve compatibility with strict V2 decoders. In
install/integrationctl/agentplugins/domain/types.go:143-144, make the necessary
serialization change only if the inventory is persisted. In
install/integrationctl/agentplugins/adapters/statev2/store_test.go:90-94,
populate app data before Save so the test exercises the contract, or remove the
forbidden-key assertions if ComponentInventory is confirmed unreachable.
ac67f32 to
93ddc62
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
cli/plugin-kit-ai/internal/agentpluginscli/source.go (1)
167-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive the hints their own clone.
Lines 172-174 assign one cloned map to both
envelope.CatalogEvidence.Compatibilityandhints.Compatibility. The two fields then share the map and the same*CatalogAppBindingvalues.catalog.goandcloneCatalogCompatibilityexist to keep mutable hints separate from immutable evidence, andcatalog_test.goasserts that separation. Nothing mutates the hints today, so this is a consistency fix rather than an active defect.♻️ Proposed change
evidence := *binding.PackageRevision.CatalogEvidence evidence.Compatibility = cloneCatalogCompatibility(evidence.Compatibility) loaded.envelope.CatalogEvidence = &evidence - loaded.hints.Compatibility = cloneCatalogCompatibility(evidence.Compatibility) + loaded.hints.Compatibility = cloneCatalogCompatibility(binding.PackageRevision.CatalogEvidence.Compatibility)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/plugin-kit-ai/internal/agentpluginscli/source.go` around lines 167 - 175, Update restoreCatalogEvidence to clone CatalogEvidence.Compatibility separately for loaded.envelope.CatalogEvidence and loaded.hints.Compatibility, ensuring both maps and their CatalogAppBinding values are independent. Reuse cloneCatalogCompatibility for each assignment and preserve the existing nil guards and restoration flow.install/integrationctl/agentplugins/adapters/catalog/catalog_test.go (1)
121-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse CLI version 0.1.6 so the subtests prove app-binding rejection.
The fixture is a schema v2 catalog.
Loadrejects schema v2 for any CLI below 0.1.6 (catalog.golines 74-76). WithCurrentCLIVersion: "0.1.0", every subtest gets a non-nil error from the version gate, so the assertionerr == nilpasses even ifValidateAppBindingaccepts the mutated binding. Raise the CLI version and assert the error text to keep the subtests meaningful.♻️ Proposed test hardening
name, body := name, body t.Run(name, func(t *testing.T) { - if _, err := (Loader{CurrentCLIVersion: "0.1.0"}).Load([]byte(body), ""); err == nil { + err := (Loader{CurrentCLIVersion: "0.1.6"}).Load + if _, loadErr := err([]byte(body), ""); loadErr == nil { t.Fatal("invalid ChatGPT app binding accepted") + } else if strings.Contains(loadErr.Error(), "0.1.6 or newer") { + t.Fatalf("rejection came from the CLI version gate, not binding validation: %v", loadErr) } })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/integrationctl/agentplugins/adapters/catalog/catalog_test.go` around lines 121 - 139, Update TestCatalogRejectsUnsafeOrMisplacedChatGPTAppBinding to use CurrentCLIVersion "0.1.6", allowing schema v2 validation to reach ValidateAppBinding. Strengthen each subtest to verify the returned error identifies the invalid app binding rather than merely asserting that any error occurred.cli/plugin-kit-ai/internal/agentpluginscli/lifecycle.go (1)
519-521: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReject
openaias ambiguous before checking the bound target list.
selectBoundClientbuilds only materialized bindings before theopenaicheck, soupdate --target openaican return “plugin has no materialized target” first. Move theopenairejection to the beginning of both normalize paths, or replace the duplicated checks with one target-normalizer helper that returns the rejection explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/plugin-kit-ai/internal/agentpluginscli/lifecycle.go` around lines 519 - 521, Move the case-insensitive, trimmed “openai” rejection ahead of bound-target lookup and materialized-binding validation in both normalization paths used by selectBoundClient. Ensure update --target openai consistently returns the ambiguity error before any “no materialized target” result, preferably by reusing a shared target-normalizer helper if appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/plugin-kit-ai/cmd/agentplugins/catalog-v2.json`:
- Around line 241-253: Update the chatgpt entry’s verification status based on
the runtime_evidence date: do not retain "tested" while the referenced August
10, 2026 evidence is future-dated relative to August 9, 2026. Either replace
runtime_evidence and runtime_evidence_revision with valid evidence from August 9
or earlier, or defer the compatibility entry until qualifying evidence exists.
In `@cli/plugin-kit-ai/internal/agentpluginscli/source.go`:
- Around line 135-141: Update the validation flow around
loaded.envelope.App.Declared and loaded.envelope.App.Present so a package that
declares .app.json but lacks the file returns an error before reaching the
catalog-binding synthesis at line 156. Preserve the existing exact-match
validation for present manifests and allow undeclared app components to follow
the current fallback behavior.
In `@install/integrationctl/agentplugins/adapters/statev2/store.go`:
- Around line 126-131: Update the Validate logic for standard package bindings
so a missing SchemaURI is allowed only when Package.FormatID equals
domain.FormatIDOpenAIPlugin; reject unknown and all other format IDs with the
existing validation error path, while preserving the required-field checks.
In `@install/integrationctl/agentplugins/usecase/service.go`:
- Around line 651-653: Update packageRevisionMatches to compare normalized
CatalogEvidence values by passing both revision.CatalogEvidence and
envelope.CatalogEvidence through cloneCatalogEvidence before reflect.DeepEqual.
Preserve the existing revision, tree digest, and manifest digest checks.
---
Nitpick comments:
In `@cli/plugin-kit-ai/internal/agentpluginscli/lifecycle.go`:
- Around line 519-521: Move the case-insensitive, trimmed “openai” rejection
ahead of bound-target lookup and materialized-binding validation in both
normalization paths used by selectBoundClient. Ensure update --target openai
consistently returns the ambiguity error before any “no materialized target”
result, preferably by reusing a shared target-normalizer helper if appropriate.
In `@cli/plugin-kit-ai/internal/agentpluginscli/source.go`:
- Around line 167-175: Update restoreCatalogEvidence to clone
CatalogEvidence.Compatibility separately for loaded.envelope.CatalogEvidence and
loaded.hints.Compatibility, ensuring both maps and their CatalogAppBinding
values are independent. Reuse cloneCatalogCompatibility for each assignment and
preserve the existing nil guards and restoration flow.
In `@install/integrationctl/agentplugins/adapters/catalog/catalog_test.go`:
- Around line 121-139: Update
TestCatalogRejectsUnsafeOrMisplacedChatGPTAppBinding to use CurrentCLIVersion
"0.1.6", allowing schema v2 validation to reach ValidateAppBinding. Strengthen
each subtest to verify the returned error identifies the invalid app binding
rather than merely asserting that any error occurred.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 842526cd-384b-42b6-abac-a0044ba3e3d4
📒 Files selected for processing (25)
README.mdcli/plugin-kit-ai/cmd/agentplugins/catalog-v2.jsoncli/plugin-kit-ai/cmd/agentplugins/main.gocli/plugin-kit-ai/cmd/agentplugins/main_test.gocli/plugin-kit-ai/internal/agentpluginscli/add.gocli/plugin-kit-ai/internal/agentpluginscli/cli_test.gocli/plugin-kit-ai/internal/agentpluginscli/lifecycle.gocli/plugin-kit-ai/internal/agentpluginscli/source.gocli/plugin-kit-ai/internal/agentpluginscli/state_migration.goinstall/integrationctl/agentplugins/adapters/catalog/catalog.goinstall/integrationctl/agentplugins/adapters/catalog/catalog_test.goinstall/integrationctl/agentplugins/adapters/loader/app.goinstall/integrationctl/agentplugins/adapters/loader/loader.goinstall/integrationctl/agentplugins/adapters/loader/loader_test.goinstall/integrationctl/agentplugins/adapters/loader/openai_plugin.goinstall/integrationctl/agentplugins/adapters/statev2/legacy_v2.goinstall/integrationctl/agentplugins/adapters/statev2/store.goinstall/integrationctl/agentplugins/adapters/statev2/store_test.goinstall/integrationctl/agentplugins/domain/catalog.goinstall/integrationctl/agentplugins/domain/state.goinstall/integrationctl/agentplugins/providers/stager.goinstall/integrationctl/agentplugins/providers/stager_test.goinstall/integrationctl/agentplugins/usecase/service.goinstall/integrationctl/agentplugins/usecase/service_test.gonpm/agentplugins/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- install/integrationctl/agentplugins/providers/stager_test.go
- README.md
- cli/plugin-kit-ai/internal/agentpluginscli/add.go
- install/integrationctl/agentplugins/adapters/loader/openai_plugin.go
- install/integrationctl/agentplugins/providers/stager.go
- install/integrationctl/agentplugins/adapters/loader/app.go
| "chatgpt": { | ||
| "package": "projected", | ||
| "verification": "tested", | ||
| "authentication": "not_required", | ||
| "app_binding": { | ||
| "app_key": "cloudflare-docs", | ||
| "id": "plugin_asdk_app_6a78e90cf73481918ef10cdb87cd4bb4", | ||
| "mcp_server": "cloudflare-docs", | ||
| "mcp_url": "https://docs.mcp.cloudflare.com/mcp", | ||
| "runtime_evidence": "tests/e2e/results/chatgpt-cloudflare-docs-personal-app-2026-08-10.json", | ||
| "runtime_evidence_revision": "2ddbb99dd190c1792b79904f9875e6322bccd243" | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not mark future-dated evidence as tested.
The entry declares verification: "tested" but its runtime_evidence path identifies a result dated August 10, 2026. The current date is August 9, 2026.
Publish evidence that exists on or before August 9, 2026 before retaining the tested claim. Otherwise, defer this ChatGPT compatibility entry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/plugin-kit-ai/cmd/agentplugins/catalog-v2.json` around lines 241 - 253,
Update the chatgpt entry’s verification status based on the runtime_evidence
date: do not retain "tested" while the referenced August 10, 2026 evidence is
future-dated relative to August 9, 2026. Either replace runtime_evidence and
runtime_evidence_revision with valid evidence from August 9 or earlier, or defer
the compatibility entry until qualifying evidence exists.
| if loaded.envelope.App.Present { | ||
| existing, matches := loaded.envelope.App.Bindings[binding.AppKey] | ||
| if !loaded.envelope.App.Enabled || !matches || len(loaded.envelope.App.Bindings) != 1 || existing.ID != binding.ID { | ||
| return fmt.Errorf("package .app.json does not exactly match the catalog ChatGPT app binding") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Fail closed when the package declares .app.json but the file is missing.
The branch is gated on App.Present. An official package can declare "apps": "./.app.json" while the file is absent. The loader then reports App.Declared == true, App.Present == false, and the app_manifest_missing diagnostic (see loader_test.go lines 94-105). Execution falls through to line 156 and synthesizes a .app.json from the catalog binding, so a package with a missing declared component installs as if it were complete. The diagnostic is preserved, but the integrity defect no longer blocks the install.
Reject the declared-but-missing case instead of substituting catalog content.
🛡️ Proposed guard
if loaded.envelope.App.Present {
existing, matches := loaded.envelope.App.Bindings[binding.AppKey]
if !loaded.envelope.App.Enabled || !matches || len(loaded.envelope.App.Bindings) != 1 || existing.ID != binding.ID {
return fmt.Errorf("package .app.json does not exactly match the catalog ChatGPT app binding")
}
return nil
}
+ if loaded.envelope.App.Declared {
+ return fmt.Errorf("package declares .app.json but the file is missing; restore the package before installing for ChatGPT")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if loaded.envelope.App.Present { | |
| existing, matches := loaded.envelope.App.Bindings[binding.AppKey] | |
| if !loaded.envelope.App.Enabled || !matches || len(loaded.envelope.App.Bindings) != 1 || existing.ID != binding.ID { | |
| return fmt.Errorf("package .app.json does not exactly match the catalog ChatGPT app binding") | |
| } | |
| return nil | |
| } | |
| if loaded.envelope.App.Present { | |
| existing, matches := loaded.envelope.App.Bindings[binding.AppKey] | |
| if !loaded.envelope.App.Enabled || !matches || len(loaded.envelope.App.Bindings) != 1 || existing.ID != binding.ID { | |
| return fmt.Errorf("package .app.json does not exactly match the catalog ChatGPT app binding") | |
| } | |
| return nil | |
| } | |
| if loaded.envelope.App.Declared { | |
| return fmt.Errorf("package declares .app.json but the file is missing; restore the package before installing for ChatGPT") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/plugin-kit-ai/internal/agentpluginscli/source.go` around lines 135 - 141,
Update the validation flow around loaded.envelope.App.Declared and
loaded.envelope.App.Present so a package that declares .app.json but lacks the
file returns an error before reaching the catalog-binding synthesis at line 156.
Preserve the existing exact-match validation for present manifests and allow
undeclared app components to follow the current fallback behavior.
| if installation.Package.FormatID == "" || installation.Source.TreeDigest == "" || installation.Package.ManifestDigest == "" { | ||
| return fmt.Errorf("%s standard package binding is incomplete", prefix) | ||
| } | ||
| if installation.Package.FormatID == domain.FormatIDAgentPluginsV1 && installation.Package.SchemaURI == "" { | ||
| return fmt.Errorf("%s portable standard package binding has no schema URI", prefix) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Reject unsupported package format IDs.
Validate now accepts any non-empty FormatID without a SchemaURI, not only domain.FormatIDOpenAIPlugin. A malformed state record with FormatID: "unknown" passes validation and can enter repair or lifecycle flows.
Allow schema-free bindings only for domain.FormatIDOpenAIPlugin. Reject all other format IDs.
Proposed fix
if installation.Package.LoaderKind == domain.LoaderKindAgentPlugins {
if installation.Package.FormatID == "" || installation.Source.TreeDigest == "" || installation.Package.ManifestDigest == "" {
return fmt.Errorf("%s standard package binding is incomplete", prefix)
}
- if installation.Package.FormatID == domain.FormatIDAgentPluginsV1 && installation.Package.SchemaURI == "" {
- return fmt.Errorf("%s portable standard package binding has no schema URI", prefix)
+ switch installation.Package.FormatID {
+ case domain.FormatIDAgentPluginsV1:
+ if installation.Package.SchemaURI == "" {
+ return fmt.Errorf("%s portable standard package binding has no schema URI", prefix)
+ }
+ case domain.FormatIDOpenAIPlugin:
+ default:
+ return fmt.Errorf("%s has unsupported standard package format %q", prefix, installation.Package.FormatID)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if installation.Package.FormatID == "" || installation.Source.TreeDigest == "" || installation.Package.ManifestDigest == "" { | |
| return fmt.Errorf("%s standard package binding is incomplete", prefix) | |
| } | |
| if installation.Package.FormatID == domain.FormatIDAgentPluginsV1 && installation.Package.SchemaURI == "" { | |
| return fmt.Errorf("%s portable standard package binding has no schema URI", prefix) | |
| } | |
| if installation.Package.FormatID == "" || installation.Source.TreeDigest == "" || installation.Package.ManifestDigest == "" { | |
| return fmt.Errorf("%s standard package binding is incomplete", prefix) | |
| } | |
| switch installation.Package.FormatID { | |
| case domain.FormatIDAgentPluginsV1: | |
| if installation.Package.SchemaURI == "" { | |
| return fmt.Errorf("%s portable standard package binding has no schema URI", prefix) | |
| } | |
| case domain.FormatIDOpenAIPlugin: | |
| default: | |
| return fmt.Errorf("%s has unsupported standard package format %q", prefix, installation.Package.FormatID) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install/integrationctl/agentplugins/adapters/statev2/store.go` around lines
126 - 131, Update the Validate logic for standard package bindings so a missing
SchemaURI is allowed only when Package.FormatID equals
domain.FormatIDOpenAIPlugin; reject unknown and all other format IDs with the
existing validation error path, while preserving the required-field checks.
| func packageRevisionMatches(revision *domain.ClientPackageRevision, envelope domain.PackageEnvelope) bool { | ||
| return revision != nil && revision.TreeDigest == envelope.TreeDigest && revision.ManifestDigest == envelope.ManifestDigest | ||
| return revision != nil && revision.TreeDigest == envelope.TreeDigest && revision.ManifestDigest == envelope.ManifestDigest && | ||
| reflect.DeepEqual(revision.CatalogEvidence, envelope.CatalogEvidence) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Normalize catalog evidence before the deep comparison.
reflect.DeepEqual treats a nil map and an empty non-nil map as different. CatalogEvidence.Compatibility uses json:"compatibility,omitempty", so an empty map is dropped on save and reloads as nil. If an in-memory envelope ever carries an empty non-nil Compatibility, the persisted revision and the envelope compare unequal forever. add then fails with "already materialized ... at a different revision; use update" on every retry.
Compare cloned values so both sides pass through the same normalization.
🛡️ Proposed fix
func packageRevisionMatches(revision *domain.ClientPackageRevision, envelope domain.PackageEnvelope) bool {
return revision != nil && revision.TreeDigest == envelope.TreeDigest && revision.ManifestDigest == envelope.ManifestDigest &&
- reflect.DeepEqual(revision.CatalogEvidence, envelope.CatalogEvidence)
+ reflect.DeepEqual(cloneCatalogEvidence(revision.CatalogEvidence), cloneCatalogEvidence(envelope.CatalogEvidence))
}cloneCatalogEvidence already leaves Compatibility nil when the source map is empty, so both sides normalize identically.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install/integrationctl/agentplugins/usecase/service.go` around lines 651 -
653, Update packageRevisionMatches to compare normalized CatalogEvidence values
by passing both revision.CatalogEvidence and envelope.CatalogEvidence through
cloneCatalogEvidence before reflect.DeepEqual. Preserve the existing revision,
tree digest, and manifest digest checks.
93ddc62 to
1565a93
Compare
1 participant
Summary
Tests
The isolated lifecycle used catalog v2 digest sha256:66199c87bd68c65e39d15aa2c5c6e6c7830c9b116d8ed3590123031b32357050. It verified zero mutation on checksum failure and dry-run, exact app ID and MCP URL projection, client-revision evidence repair, State v3 persistence, and final removal. No real client configuration or runtime was touched.
One full CLI run hit the pre-existing parallel LookPath test race; its isolated rerun and a complete CLI rerun passed. Full make test-required previously passed root, repotests, CLI, and every changed agentplugins package. Its only failures were pre-existing OpenCode tests reproduced unchanged in a detached pristine origin/main worktree at 2404e01.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation