feat(agentplugins): harden lifecycle and release E2E - #47
Conversation
|
Warning Review limit reached
Next review available in: 52 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 (1)
📝 WalkthroughWalkthroughThe pull request adds immutable release and publication gates, six-platform runtime proofs, stricter catalog and client verification, lifecycle resumption, transactional repair, and expanded CLI diagnostics and tests. ChangesRelease validation and publication
Catalog and installer behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ReleaseWorkflow
participant PlatformProof
participant NativeRuntimes
participant NpmPublish
ReleaseWorkflow->>PlatformProof: stage and verify the immutable draft
PlatformProof->>NativeRuntimes: run six native runtime proofs
NativeRuntimes-->>PlatformProof: return proof artifacts
PlatformProof-->>ReleaseWorkflow: return gate eligibility and tested tarball
ReleaseWorkflow->>NpmPublish: pass the verified release identity and tarball
NpmPublish-->>ReleaseWorkflow: publish the exact tested tarball
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install/integrationctl/agentplugins/planner/planner.go (1)
100-119: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not append activation guidance to an unsupported plan.
applyCatalogCompatibilitycan setplan.Status = domain.PlanUnsupportedandplan.Activation = domain.ActivationFailedat Line 130 and Line 146, and the function does not return early. The switch at Lines 100-119 then still appends client guidance such as "finish installation in Codex or ChatGPT Plugins, then start a new session". The user receives both "choose a client present in the pinned catalog evidence" and instructions to finish an installation that the planner refused. The same happens for theno_valid_componentsandno_supported_componentspaths at Lines 86-94.Skip the client-specific guidance when the plan is unsupported.
🛠️ Proposed fix
- switch client.ClientID { + if plan.Status == domain.PlanUnsupported { + return plan, nil + } + switch client.ClientID { case domain.ClientCodex:🤖 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/planner/planner.go` around lines 100 - 119, Skip the client-specific guidance switch when the plan has status domain.PlanUnsupported, including plans made unsupported by no_valid_components, no_supported_components, or applyCatalogCompatibility. Ensure unsupported plans retain only their existing unsupported-plan messaging and do not append activation instructions.
🧹 Nitpick comments (18)
npm/agentplugins/test/stage-release.test.js (1)
86-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the rejected legacy manifest shape.
verifyReleasealso throwslegacy release manifest has unexpected fieldswhen a schema-1 manifest carries extra keys. That branch is currently untested. Add one case that writes a schema-1 manifest with an extra key and asserts the throw, so the audit path stays strict.🤖 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 `@npm/agentplugins/test/stage-release.test.js` around lines 86 - 110, Extend the legacy manifest test around verifyRelease with a schema-1 manifest containing an unexpected extra field, then assert that the allowLegacyManifest audit path throws the “legacy release manifest has unexpected fields” error. Keep the existing valid schema-1 audit assertions unchanged..github/workflows/agentplugins-platform-proof.yml (1)
244-252: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winBind workflow expressions through
envin everyrunblock. Four steps expand${{ ... }}directly into shell script bodies. Most of these steps already declare anenv:block for other values, so the pattern is inconsistent. Binding each value to an environment variable keeps the script text static, removes the shell-quoting dependency on the expression value, and clears the zizmortemplate-injectionfindings.
.github/workflows/agentplugins-platform-proof.yml#L244-L252: bindneeds.prepare.outputs.tarball_file,needs.prepare.outputs.version,matrix.target,matrix.lifecycle, andrunner.temptoenvand reference them as shell variables in theplatform-proof.jsinvocation..github/workflows/agentplugins-release.yml#L229-L250: addFROZEN_COMMIT: ${{ needs.validate.outputs.commit }}to the existingenvblock and use--target "${FROZEN_COMMIT}"..github/workflows/agentplugins-npm-publish.yml#L92-L97: addFROZEN_COMMITandGATE_ELIGIBLEto the existingenvblock and compare against those variables..github/workflows/agentplugins-npm-publish.yml#L129-L136: addRELEASE_COMMIT: ${{ steps.publish-gate.outputs.commit }}to the existingenvblock and use it for both therelease-assets.js verifyargument and--source-digest.🤖 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 @.github/workflows/agentplugins-platform-proof.yml around lines 244 - 252, Replace direct workflow-expression interpolation in all four sites with environment-variable bindings: in .github/workflows/agentplugins-platform-proof.yml lines 244-252, bind the tarball file, version, target, lifecycle, and runner temp values and reference shell variables in the platform-proof.js command; in .github/workflows/agentplugins-release.yml lines 229-250, add FROZEN_COMMIT to the existing env block and use it for --target; in .github/workflows/agentplugins-npm-publish.yml lines 92-97, bind FROZEN_COMMIT and GATE_ELIGIBLE and compare those variables; and in lines 129-136, bind RELEASE_COMMIT and use it for both release-assets.js verify and --source-digest.Source: Linters/SAST tools
npm/agentplugins/scripts/platform-proof.js (1)
104-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the installed package directory from the tarball manifest.
Line 104 hardcodes
universal-agent-plugins. The publish workflow resolves the package name frompackage.jsonand passes it asEXPECTED_PACKAGE. If the distribution name changes, this proof fails with an unclearENOENTinstead of a named identity error. Consider accepting the expected package name as an argument, or reading it from the projectpackage.jsondependencies after install.🤖 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 `@npm/agentplugins/scripts/platform-proof.js` around lines 104 - 111, The platform proof currently hardcodes the installed directory name in the packageRoot construction. Update the surrounding proof entrypoint to use the expected package name supplied by the publish workflow, such as EXPECTED_PACKAGE or an equivalent argument, and construct packageRoot from that value. Preserve the existing version, script, and manifest validations while ensuring a missing or mismatched package identity produces the named validation failure rather than an unclear ENOENT..github/workflows/agentplugins-release.yml (1)
281-289: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert
platform-proofgate eligibility before promotion.The npm publish workflow checks
needs.platform-proof.outputs.gate_eligibleat line 95 ofagentplugins-npm-publish.yml. This job does not. Todayallow_legacy_manifest: falsemakes a non-eligible outcome impossible, so the gap is latent rather than exploitable. Add the explicit check so the promotion gate does not depend on that indirect argument.♻️ Proposed change
env: GH_TOKEN: ${{ github.token }} TAG: ${{ inputs.tag }} FROZEN_COMMIT: ${{ needs.validate.outputs.commit }} EXPECTED_ASSET_SET_DIGEST: ${{ needs.stage-draft.outputs.asset_set_digest }} + GATE_ELIGIBLE: ${{ needs.platform-proof.outputs.gate_eligible }} run: | + test "${GATE_ELIGIBLE}" = "true" + [[ "${EXPECTED_ASSET_SET_DIGEST}" =~ ^[0-9a-f]{64}$ ]] || { + echo "missing frozen asset-set digest from the staging job" >&2 + exit 1 + } git fetch --force origin "refs/tags/${TAG}:refs/tags/${TAG}"🤖 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 @.github/workflows/agentplugins-release.yml around lines 281 - 289, Update the “Reverify exact draft and promote only after all native proofs” job to explicitly validate needs.platform-proof.outputs.gate_eligible before promotion, matching the guard used by the npm publish workflow. Fail the job when the value is not eligible, while preserving the existing tag and frozen-commit verification.install/integrationctl/agentplugins/ports/interfaces.go (1)
27-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider keeping the classification in the message.
When
Erris set,Error()returns only the wrapped message and dropsKind. Logs then lose the classification that this type exists to carry. Wrapping the kind keepserrors.Isanderrors.Asbehavior throughUnwrapand improves the log text.♻️ Proposed change
func (err *VerificationError) Error() string { if err.Err != nil { - return err.Err.Error() + return fmt.Sprintf("%s: %s", err.Kind, err.Err.Error()) } return fmt.Sprintf("package verification failed: %s", err.Kind) }🤖 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/ports/interfaces.go` around lines 27 - 32, Update VerificationError.Error to include err.Kind in the message even when err.Err is non-nil, while preserving the wrapped error details and existing Unwrap behavior so errors.Is and errors.As continue to work.install/integrationctl/agentplugins/planner/planner.go (1)
81-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a tagged switch on
plan.Authentication.staticcheck reports QF1003 here. A tagged switch also makes new
AuthenticationStatevalues easier to add.♻️ Proposed change
- if plan.Authentication == domain.AuthenticationPending { - plan.UserActions = append(plan.UserActions, "complete authentication for this plugin in the selected client") - } else if plan.Authentication == domain.AuthenticationNotChecked { - plan.UserActions = append(plan.UserActions, "verify the plugin's authentication requirements before using it") - } + switch plan.Authentication { + case domain.AuthenticationPending: + plan.UserActions = append(plan.UserActions, "complete authentication for this plugin in the selected client") + case domain.AuthenticationNotChecked: + plan.UserActions = append(plan.UserActions, "verify the plugin's authentication requirements before using it") + }🤖 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/planner/planner.go` around lines 81 - 85, Replace the if/else chain keyed by plan.Authentication with a tagged switch on plan.Authentication, preserving the existing UserActions append behavior for AuthenticationPending and AuthenticationNotChecked and leaving other authentication states unchanged.Source: Linters/SAST tools
install/integrationctl/agentplugins/providers/activator.go (2)
428-428: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the ST1005 lint failure on the error string.
golangci-lint reports ST1005 for the capitalized error string. Lowercase the first word to pass the linter. The same pattern exists at Line 563 and Line 647; align all three.
🔧 Proposed fix
-var errCodexListContractUnknown = errors.New("Codex plugin list output is not recognized") +var errCodexListContractUnknown = errors.New("codex plugin list output is not recognized")🤖 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/providers/activator.go` at line 428, Lowercase the first word in the error strings for errCodexListContractUnknown and the corresponding error declarations near the other two reported locations, preserving the rest of each message so all three satisfy ST1005.Source: Linters/SAST tools
434-435: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRead the bytes directly.
strings.NewReader(string(body))copies the whole payload.bytes.NewReader(body)avoids the copy and reads the same content.♻️ Proposed change
- decoder := json.NewDecoder(strings.NewReader(string(body))) + decoder := json.NewDecoder(bytes.NewReader(body))Add the
bytesimport if it is not already present.🤖 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/providers/activator.go` around lines 434 - 435, Update the decoder initialization near UseNumber to pass body directly through bytes.NewReader instead of converting it to a string first, and add the bytes import if needed. Preserve the existing JSON decoding behavior.cli/plugin-kit-ai/internal/agentpluginscli/source.go (1)
99-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one shared clone helper.
cloneCatalogCompatibilityduplicatescloneCompatibilityininstall/integrationctl/agentplugins/adapters/catalog/catalog.go. Both functions have identical behavior. Export one helper from thedomainpackage and call it from both sites. This keeps the copy semantics in one place ifCatalogCompatibilitygains reference-typed fields later.🤖 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 99 - 108, Replace the duplicate cloneCatalogCompatibility and cloneCompatibility implementations with one exported clone helper in the domain package. Update both source.go and the catalog adapter to call that shared helper, preserving the current nil-for-empty behavior and shallow-copy semantics.install/integrationctl/agentplugins/adapters/catalog/catalog_test.go (1)
114-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the hardcoded embedded catalog size assertion.
The test should assert the
Loadresult is non-empty and valid; an exact plugin count makes the test fail on future catalog additions. The matrix invariant is already enforced duringLoad, including rejection of empty plugin lists.♻️ Proposed change
- if len(loaded.Catalog.Plugins) != 26 { - t.Fatalf("embedded package count = %d, want 26", len(loaded.Catalog.Plugins)) + if len(loaded.Catalog.Plugins) == 0 { + t.Fatal("embedded catalog contains no plugins") }🤖 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 114 - 127, Update TestEmbeddedCatalogAllEntriesUseExactCompatibilityMatrix to remove the exact len(loaded.Catalog.Plugins) == 26 assertion. Instead, assert that the Load result is valid and contains a non-empty plugin list, relying on Loader.Load for compatibility-matrix validation and empty-list rejection.install/integrationctl/agentplugins/usecase/repair.go (2)
109-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
time.RFC3339Nanoinstead of the inline layout.Both lines repeat the literal
"2006-01-02T15:04:05.999999999Z07:00".service.updateLifecycleusestime.RFC3339Nanofor the same field. Use the constant so all timestamp writers stay identical.Also applies to: 162-162
🤖 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/repair.go` at line 109, Update the timestamp formatting in service.updateLifecycle and the other corrected.UpdatedAt assignment to use time.RFC3339Nano instead of the repeated inline layout, keeping all lifecycle timestamp writers consistent.
74-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the inner
verifyErrto remove the shadow.Line 76 declares a second
verifyErrinside theverifyErr == nilblock. The outerverifyErris still used at line 120 after the block. The current code is correct, but the shadow makes later edits inside the block risky. Use a distinct name, for exampleclientErr.♻️ Proposed rename
- verified, verifyErr := service.verifyClientReadOnly(ctx, input, result, client) - if verifyErr != nil { - return result, verifyErr - } + verified, clientErr := service.verifyClientReadOnly(ctx, input, result, client) + if clientErr != nil { + return result, clientErr + }🤖 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/repair.go` around lines 74 - 79, Rename the inner error variable returned by service.verifyClientReadOnly in the verifyErr == nil block to a distinct name such as clientErr, and update its nil check and return accordingly; preserve the outer verifyErr for later use.cli/plugin-kit-ai/internal/agentpluginscli/lifecycle.go (1)
80-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExplicitly discard the
cleanuperror.golangci-lint reports an unchecked error return on line 81. Assign the result to the blank identifier so the deferred call states the intent.
🔧 Proposed fix
if loaded.cleanup != nil { - defer loaded.cleanup() + defer func() { _ = loaded.cleanup() }() }🤖 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 80 - 82, Update the deferred loaded.cleanup call in the lifecycle cleanup block to explicitly assign its returned error to the blank identifier, preserving the existing defer behavior while satisfying unchecked-error linting.Source: Linters/SAST tools
install/integrationctl/agentplugins/usecase/service_test.go (2)
174-191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the map-indexed conditional with a plain
if.
map[bool]string{true: "projected", false: "native"}[clientID == domain.ClientCodex]allocates a map to pick one of two strings. A smallpackageMode := "native"with anifis clearer and matches the rest of the file.🤖 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_test.go` around lines 174 - 191, The test setup in TestOpenAIOAuthHintsDoNotOverrideGenericAuthentication uses a map-indexed conditional to select the package value. Replace it with a packageMode variable initialized to "native" and a plain if that assigns "projected" for ClientCodex, then use packageMode in CatalogEvidence.
85-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe evidence assertions test the input, not the persisted state.
input.Envelope.Manifest.Raw,input.Envelope.CatalogEvidenceandinput.Envelope.Diagnosticsare values the test itself set at lines 61-66. Those three conditions always hold, so they cannot detect evidence loss. OnlypackageState.SchemaURIandpackageState.ManifestDigestread persisted state. Assert against the reloaded state, or against the returned result, for the evidence fields you care about.🤖 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_test.go` around lines 85 - 89, Update the assertions in the test around packageState and the returned or reloaded persisted state so evidence fields are validated from that state rather than input.Envelope. Replace the self-referential checks on Manifest.Raw, CatalogEvidence, and Diagnostics with assertions against the persisted result, while preserving the existing SchemaURI and ManifestDigest checks.install/integrationctl/agentplugins/usecase/service.go (2)
360-380: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTwo different definitions of "a verifier is available" exist for the same clients.
clientVerifierAvailablerequires, fordomain.ClientKiro, a kiro executable and MCP-server-only supported components.verifyClientReadOnlyrequires only the kiro executable. The two functions therefore disagree for a Kiro plan that has no components or has unsupported components:resumetreats the verifier as unavailable and preserves the recorded lifecycle, while the converged path still callsActivateand can overwrite that lifecycle from the same client. Derive both from one predicate.Also applies to: 499-511
🤖 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 360 - 380, Unify the Kiro verifier-availability logic used by clientVerifierAvailable and verifyClientReadOnly into one shared predicate. Preserve the Kiro executable requirement and MCP-server component validation consistently, including the no-components and unsupported-component cases, so both paths make the same availability decision before activation or lifecycle preservation.
143-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the converged-verification block; the two copies can drift.
Lines 143-172 and 176-204 run the same sequence:
lifecycleConverged,verifyClientReadOnly, authoritative persistence, no-change detection,resume. Only the binding source differs (currentversuspreviousClient). Extract one helper that takes the binding and returns(AddResult, error). That keeps the fail-closed persistence rule in a single place.Also add parentheses in the persistence condition.
input.Confirmed || input.PersistAuthoritativeObservations && verified.AuthoritativeObservationreads as three alternatives, but&&binds first.🤖 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 143 - 204, Extract the duplicated converged-verification flow from the materialized add and replace paths into one helper that accepts the client binding and returns (AddResult, error), preserving lifecycle verification, authoritative-observation persistence, lifecycle updates, no-change handling, and resume behavior. Replace both inline blocks with calls to this helper for current and previousClient. In the authoritative persistence condition, add parentheses around the PersistAuthoritativeObservations and verified.AuthoritativeObservation conjunction.install/integrationctl/agentplugins/adapters/statev2/store_test.go (1)
25-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLegacy shape reuses current domain types, so nested fields escape the strict check.
oldInstallationV2embedsdomain.SourceBindinganddomain.ClientBinding. These are the current types.DisallowUnknownFieldstherefore accepts any new field added inside a client binding or source binding. The test then only proves compatibility at the top two levels. Declare frozen local copies of the 0.1.4ClientBindingandSourceBindingshapes to close that gap.🤖 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_test.go` around lines 25 - 34, Replace the domain.SourceBinding and domain.ClientBinding fields used by oldInstallationV2 with local frozen 0.1.4-compatible copies, including their nested field types and JSON tags. Update the legacy test fixtures and unmarshalling path to use these local ClientBinding and SourceBinding definitions so DisallowUnknownFields validates nested fields against the historical schema.
🤖 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 @.github/workflows/agentplugins-platform-proof.yml:
- Around line 123-126: Update the commit validation around EXPECTED_COMMIT so
workflow_call executions reject an empty value instead of skipping the check;
require a non-empty, valid commit-format value before comparing commit, while
preserving the existing mismatch failure and message.
- Around line 262-268: Update the proof-complete job’s “Confirm native matrix
aggregation” step to download the agentplugins-native-proof-* artifacts and
validate exactly one proof document for each of the six required native targets.
Retain the native-runtime success check, but make the job fail when any target
artifact or document is missing or the matrix produces fewer than six valid
proofs.
- Around line 73-76: Update the workflow permissions to grant contents: write so
the require_draft path can access draft release assets through GITHUB_TOKEN.
Preserve the existing draft checks and release download verification behavior.
In `@cli/plugin-kit-ai/internal/agentpluginscli/lifecycle.go`:
- Around line 97-107: Update the unconfirmed branch in the repair lifecycle flow
to preserve the JSON output contract: when opts.format is "json", render the
appropriate JSON result instead of printing the plain-text "No changes made."
message. Keep the existing human-readable output for other formats and return
without applying changes, matching the behavior implemented by runUpdate.
- Around line 47-96: Use a single consistent stdin reader across
selectBoundClient and the later repair confirmation in runRepair, rather than
creating separate buffered readers that can consume each other’s input. Pass
ownership of one reader through the interactive selection and promptYesNo flow,
or make the chooser use the same unbuffered line-reading mechanism, while
preserving existing non-interactive and dry-run behavior.
In `@cli/plugin-kit-ai/internal/agentpluginscli/read.go`:
- Around line 236-242: Update the Copilot-specific check in the binding findings
logic to require visible before evaluating client.ExecutablePath. Keep
copilot_cli_missing for detected clients with an empty executable path, while
preserving client_not_visible for bindings absent from detectedByID or otherwise
not visible.
In `@install/integrationctl/agentplugins/adapters/clientdetect/detector.go`:
- Around line 41-45: Update the LinuxApplicationDirs construction to include the
user-local applications path only when homeDir is non-blank; otherwise retain
only the system application directories and avoid probing a relative
.local/share/applications path. Use the existing compactPaths setup and nearby
detector logic without changing system paths.
In `@npm/agentplugins/scripts/platform-proof.js`:
- Around line 40-43: Update the argument validation around lifecycleArg in the
platform-proof.js entrypoint to accept only the exact literals "true" or
"false"; reject any other value through fail(...) before the lifecycle proof
logic interprets it, while preserving the existing required-argument checks and
valid-value behavior.
- Around line 97-103: Quote the tarball argument in the npm install invocation
when running on Windows with shell enabled, so paths containing spaces remain a
single argument; preserve the unquoted path for non-Windows execution. Update
the argument construction around tarball and the existing run call.
---
Outside diff comments:
In `@install/integrationctl/agentplugins/planner/planner.go`:
- Around line 100-119: Skip the client-specific guidance switch when the plan
has status domain.PlanUnsupported, including plans made unsupported by
no_valid_components, no_supported_components, or applyCatalogCompatibility.
Ensure unsupported plans retain only their existing unsupported-plan messaging
and do not append activation instructions.
---
Nitpick comments:
In @.github/workflows/agentplugins-platform-proof.yml:
- Around line 244-252: Replace direct workflow-expression interpolation in all
four sites with environment-variable bindings: in
.github/workflows/agentplugins-platform-proof.yml lines 244-252, bind the
tarball file, version, target, lifecycle, and runner temp values and reference
shell variables in the platform-proof.js command; in
.github/workflows/agentplugins-release.yml lines 229-250, add FROZEN_COMMIT to
the existing env block and use it for --target; in
.github/workflows/agentplugins-npm-publish.yml lines 92-97, bind FROZEN_COMMIT
and GATE_ELIGIBLE and compare those variables; and in lines 129-136, bind
RELEASE_COMMIT and use it for both release-assets.js verify and --source-digest.
In @.github/workflows/agentplugins-release.yml:
- Around line 281-289: Update the “Reverify exact draft and promote only after
all native proofs” job to explicitly validate
needs.platform-proof.outputs.gate_eligible before promotion, matching the guard
used by the npm publish workflow. Fail the job when the value is not eligible,
while preserving the existing tag and frozen-commit verification.
In `@cli/plugin-kit-ai/internal/agentpluginscli/lifecycle.go`:
- Around line 80-82: Update the deferred loaded.cleanup call in the lifecycle
cleanup block to explicitly assign its returned error to the blank identifier,
preserving the existing defer behavior while satisfying unchecked-error linting.
In `@cli/plugin-kit-ai/internal/agentpluginscli/source.go`:
- Around line 99-108: Replace the duplicate cloneCatalogCompatibility and
cloneCompatibility implementations with one exported clone helper in the domain
package. Update both source.go and the catalog adapter to call that shared
helper, preserving the current nil-for-empty behavior and shallow-copy
semantics.
In `@install/integrationctl/agentplugins/adapters/catalog/catalog_test.go`:
- Around line 114-127: Update
TestEmbeddedCatalogAllEntriesUseExactCompatibilityMatrix to remove the exact
len(loaded.Catalog.Plugins) == 26 assertion. Instead, assert that the Load
result is valid and contains a non-empty plugin list, relying on Loader.Load for
compatibility-matrix validation and empty-list rejection.
In `@install/integrationctl/agentplugins/adapters/statev2/store_test.go`:
- Around line 25-34: Replace the domain.SourceBinding and domain.ClientBinding
fields used by oldInstallationV2 with local frozen 0.1.4-compatible copies,
including their nested field types and JSON tags. Update the legacy test
fixtures and unmarshalling path to use these local ClientBinding and
SourceBinding definitions so DisallowUnknownFields validates nested fields
against the historical schema.
In `@install/integrationctl/agentplugins/planner/planner.go`:
- Around line 81-85: Replace the if/else chain keyed by plan.Authentication with
a tagged switch on plan.Authentication, preserving the existing UserActions
append behavior for AuthenticationPending and AuthenticationNotChecked and
leaving other authentication states unchanged.
In `@install/integrationctl/agentplugins/ports/interfaces.go`:
- Around line 27-32: Update VerificationError.Error to include err.Kind in the
message even when err.Err is non-nil, while preserving the wrapped error details
and existing Unwrap behavior so errors.Is and errors.As continue to work.
In `@install/integrationctl/agentplugins/providers/activator.go`:
- Line 428: Lowercase the first word in the error strings for
errCodexListContractUnknown and the corresponding error declarations near the
other two reported locations, preserving the rest of each message so all three
satisfy ST1005.
- Around line 434-435: Update the decoder initialization near UseNumber to pass
body directly through bytes.NewReader instead of converting it to a string
first, and add the bytes import if needed. Preserve the existing JSON decoding
behavior.
In `@install/integrationctl/agentplugins/usecase/repair.go`:
- Line 109: Update the timestamp formatting in service.updateLifecycle and the
other corrected.UpdatedAt assignment to use time.RFC3339Nano instead of the
repeated inline layout, keeping all lifecycle timestamp writers consistent.
- Around line 74-79: Rename the inner error variable returned by
service.verifyClientReadOnly in the verifyErr == nil block to a distinct name
such as clientErr, and update its nil check and return accordingly; preserve the
outer verifyErr for later use.
In `@install/integrationctl/agentplugins/usecase/service_test.go`:
- Around line 174-191: The test setup in
TestOpenAIOAuthHintsDoNotOverrideGenericAuthentication uses a map-indexed
conditional to select the package value. Replace it with a packageMode variable
initialized to "native" and a plain if that assigns "projected" for ClientCodex,
then use packageMode in CatalogEvidence.
- Around line 85-89: Update the assertions in the test around packageState and
the returned or reloaded persisted state so evidence fields are validated from
that state rather than input.Envelope. Replace the self-referential checks on
Manifest.Raw, CatalogEvidence, and Diagnostics with assertions against the
persisted result, while preserving the existing SchemaURI and ManifestDigest
checks.
In `@install/integrationctl/agentplugins/usecase/service.go`:
- Around line 360-380: Unify the Kiro verifier-availability logic used by
clientVerifierAvailable and verifyClientReadOnly into one shared predicate.
Preserve the Kiro executable requirement and MCP-server component validation
consistently, including the no-components and unsupported-component cases, so
both paths make the same availability decision before activation or lifecycle
preservation.
- Around line 143-204: Extract the duplicated converged-verification flow from
the materialized add and replace paths into one helper that accepts the client
binding and returns (AddResult, error), preserving lifecycle verification,
authoritative-observation persistence, lifecycle updates, no-change handling,
and resume behavior. Replace both inline blocks with calls to this helper for
current and previousClient. In the authoritative persistence condition, add
parentheses around the PersistAuthoritativeObservations and
verified.AuthoritativeObservation conjunction.
In `@npm/agentplugins/scripts/platform-proof.js`:
- Around line 104-111: The platform proof currently hardcodes the installed
directory name in the packageRoot construction. Update the surrounding proof
entrypoint to use the expected package name supplied by the publish workflow,
such as EXPECTED_PACKAGE or an equivalent argument, and construct packageRoot
from that value. Preserve the existing version, script, and manifest validations
while ensuring a missing or mismatched package identity produces the named
validation failure rather than an unclear ENOENT.
In `@npm/agentplugins/test/stage-release.test.js`:
- Around line 86-110: Extend the legacy manifest test around verifyRelease with
a schema-1 manifest containing an unexpected extra field, then assert that the
allowLegacyManifest audit path throws the “legacy release manifest has
unexpected fields” error. Keep the existing valid schema-1 audit assertions
unchanged.
🪄 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: ad5361f3-c5cb-4233-85b4-13f55c6324e5
📒 Files selected for processing (35)
.github/workflows/agentplugins-npm-publish.yml.github/workflows/agentplugins-platform-proof.yml.github/workflows/agentplugins-release.ymlcli/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/read.gocli/plugin-kit-ai/internal/agentpluginscli/root.gocli/plugin-kit-ai/internal/agentpluginscli/source.godocs/agentplugins-release.mdinstall/integrationctl/agentplugins/adapters/catalog/catalog.goinstall/integrationctl/agentplugins/adapters/catalog/catalog_test.goinstall/integrationctl/agentplugins/adapters/clientdetect/detector.goinstall/integrationctl/agentplugins/adapters/clientdetect/detector_test.goinstall/integrationctl/agentplugins/adapters/loader/loader.goinstall/integrationctl/agentplugins/adapters/statev2/store_test.goinstall/integrationctl/agentplugins/domain/catalog.goinstall/integrationctl/agentplugins/domain/clients.goinstall/integrationctl/agentplugins/domain/types.goinstall/integrationctl/agentplugins/planner/planner.goinstall/integrationctl/agentplugins/planner/planner_test.goinstall/integrationctl/agentplugins/ports/interfaces.goinstall/integrationctl/agentplugins/providers/activator.goinstall/integrationctl/agentplugins/providers/activator_test.goinstall/integrationctl/agentplugins/providers/stager.goinstall/integrationctl/agentplugins/usecase/repair.goinstall/integrationctl/agentplugins/usecase/service.goinstall/integrationctl/agentplugins/usecase/service_test.gonpm/agentplugins/package.jsonnpm/agentplugins/scripts/platform-proof.jsnpm/agentplugins/scripts/release-assets.jsnpm/agentplugins/scripts/stage-release.jsnpm/agentplugins/test/bootstrap.test.jsnpm/agentplugins/test/stage-release.test.jsrepotests/agentplugins_release_contract_test.go
| permissions: | ||
| contents: read | ||
| attestations: read | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does GITHUB_TOKEN with contents read permission allow downloading draft release assets
💡 Result:
No, a GITHUB_TOKEN with only contents: read permission does not allow you to download assets from a draft release [1][2]. GitHub treats draft releases as non-public, "unpublished" resources [3]. To make draft releases visible and accessible to the GITHUB_TOKEN, the token must have contents: write permission [1][2][3]. If you attempt to use the GitHub CLI (gh) or API to download assets from a draft release with only contents: read, you will typically receive a 404 Not Found error (indicating the release or asset could not be found) [1][2][4] or a 403 Forbidden error because the integration does not have the necessary privilege to see the draft [1]. Even though the operation is technically a "read" action, the elevated contents: write permission is required to bypass the visibility restrictions applied to draft releases [1][2][5].
Citations:
- 1: Accessing draft releases is not possible using GITHUB_TOKEN in Actions cli/cli#3037
- 2: https://bitfreedom.net/code/apunkt/invisible_playwright/commit/7260f461bba0f96cc052a909763ed475affcc640
- 3: https://www.codegenes.net/blog/what-permissions-does-github-token-require-for-releases-from-a-github-action/
- 4: Fix draft release asset download in docker-image job github/gh-aw#10440
- 5: https://knope.tech/recipes/uploading-assets-to-draft-releases/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target workflow excerpt =="
sed -n '1,200p' .github/workflows/agentplugins-platform-proof.yml | nl -ba | sed -n '1,200p'
echo
echo "== release-related commands in workflow =="
rg -n "release|attestations|contents:|require_draft|expected_asset_set_digest|attestation verify|release download|repository\\.release" .github/workflows/agentplugins-platform-proof.ymlRepository: 777genius/plugin-kit-ai
Length of output: 231
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target workflow excerpts =="
awk '{printf "%6d\t%s\n", NR, $0}' .github/workflows/agentplugins-platform-proof.yml | sed -n '1,200p'
echo
echo "== release-related commands and permissions in workflow =="
grep -nE "release|attestations:|contents:|require_draft|attestation verify|release download|repository\.release|expected_asset_set_digest" .github/workflows/agentplugins-platform-proof.yml || trueRepository: 777genius/plugin-kit-ai
Length of output: 14391
🌐 Web query:
GitHub GraphQL Repository release draft GITHUB_TOKEN contents read permissions
💡 Result:
For a GitHub Actions workflow using the auto-generated GITHUB_TOKEN, the permissions model is controlled by the workflow/job-level permissions you set with the permissions: key, and the effective access is scoped to the repository for that job.[1][2][3] 1) Reading “draft releases” via API / GraphQL - The GitHub GraphQL “Release” type includes an isDraft field, so draft releases are represented as Release objects in GraphQL.[4] - GitHub’s Actions documentation emphasizes using permissions: to grant the minimum required access for the GITHUB_TOKEN.[5][2][3] - Practical evidence from GitHub Actions indicates that limiting the token to contents: read may be insufficient for operations involving draft releases; workflows creating/listing drafts often need contents: write.[6][7] 2) Creating/updating a draft release - A GitHub Action that creates a draft release requires the GITHUB_TOKEN to have contents: write permission.[6] 3) How to set it in your workflow Example (job-level): permissions: contents: write GitHub documents that you can set permissions at workflow or job scope to adjust the default permissions granted to GITHUB_TOKEN, and that write includes read.[2][3] 4) Important distinction: GraphQL token vs GITHUB_TOKEN - GitHub’s GraphQL docs describe authenticating to the GraphQL API using PATs or GitHub App/OAuth tokens (which must have the required permissions/scopes for the requested resources).[8] - In Actions, GITHUB_TOKEN is an installation access token created for the workflow job and expires when the job finishes; its permissions are limited to the repository containing the workflow.[1] If you paste your exact GraphQL query (or REST endpoint) and whether you’re only listing drafts vs creating/updating them, I can map it more precisely to the minimum permissions: settings.
Citations:
- 1: https://docs.github.com/en/actions/concepts/security/github_token
- 2: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax
- 3: https://docs.github.com/actions/reference/authentication-in-a-workflow
- 4: https://docs.github.com/en/graphql/reference/releases
- 5: https://docs.github.com/en/actions/tutorials/authenticate-with-github_token
- 6: https://stackoverflow.com/questions/67389957/what-permissions-does-github-token-require-for-releases-from-a-github-action
- 7: Accessing draft releases is not possible using GITHUB_TOKEN in Actions cli/cli#3037
- 8: https://github.com/github/docs/blob/c9bd77a9/content/graphql/guides/forming-calls-with-graphql.md
🌐 Web query:
GitHub CLI gh release download draft release GITHUB_TOKEN contents read write
💡 Result:
To download a draft release using the GitHub CLI (gh), you must be authenticated with a token that has sufficient permissions, specifically contents: write [1][2]. Key considerations for using gh with draft releases: 1. Permission Requirements: While contents: read is often sufficient for public/published releases, draft releases are considered private and restricted [2][3]. To access them via the GitHub CLI or API, the authentication token (such as the default GITHUB_TOKEN in GitHub Actions) must be granted contents: write permissions [1][2]. Without this, you may encounter 403 (Resource not accessible by integration) or 404 errors [2][3]. 2. Authentication: The GitHub CLI requires authentication to interact with private repository data or draft releases [4][5]. If running in GitHub Actions, you can configure the job permissions to grant the necessary access: permissions: contents: write 3. CLI Behavior: Note that there have been known issues where gh release download may fail or behave unexpectedly when targeting a draft release because the expected archive URLs (like zipball_url) are null for drafts [6]. Ensure you are using a recent version of the GitHub CLI to benefit from bug fixes related to draft release handling [6][5]. 4. Accessing Drafts: Draft releases are only visible to users (or integrations) with push access to the repository [3]. If you are using the GITHUB_TOKEN in a workflow, ensure the permissions block is defined at the job or workflow level as shown above [1][7][8].
Citations:
- 1: Accessing draft releases is not possible in GitHub runners using
gh release listcli/cli#9076 - 2: Accessing draft releases is not possible using GITHUB_TOKEN in Actions cli/cli#3037
- 3: https://docs.github.com/en/rest/releases/releases
- 4: Use gh cli to download Github Release from public repo. cli/cli#3820
- 5: Allow certain requests to be unauthenticated. cli/cli#2680
- 6:
gh release downloaderrors confusingly when release is a draft cli/cli#10797 - 7: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository
- 8: https://stackoverflow.com/questions/72383123/what-permissions-are-needed-for-github-actions-to-create-a-tag-and-release-for-a
Grant draft-release access instead of requiring both draft checks and downloadable assets.
When require_draft is true, the workflow still runs gh release download "${TAG}" on a draft release. contents: read cannot authenticate draft release assets with the workflow GITHUB_TOKEN; contents: write is required for draft release access. If the gate must stay read-only, remove the draft asset download/verification path when the release is kept as a draft.
🤖 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 @.github/workflows/agentplugins-platform-proof.yml around lines 73 - 76,
Update the workflow permissions to grant contents: write so the require_draft
path can access draft release assets through GITHUB_TOKEN. Preserve the existing
draft checks and release download verification behavior.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
npm/agentplugins/test/platform-proof.test.js (1)
74-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the on-disk byte mismatch branch of
frozenReleaseAsset.Both rejection cases assert
/manifest does not match/. Both fail in the first branch offrozenReleaseAsset, because the manifest comparison runs before the file is read. The second branch, "frozen release binary does not match the npm package size and SHA-256 pin", is never exercised. That branch is the byte-level check that binds the local file to the release pin.Add a case where the manifest and the pin agree but the file on disk holds different bytes.
♻️ Proposed additional case
assert.throws( () => frozenReleaseAsset(root, commit, version, target, { ...pinned, sha256: "0".repeat(64) }), /manifest does not match/ ); + fs.writeFileSync(path.join(root, file), Buffer.from("tampered frozen binary")); + assert.throws( + () => frozenReleaseAsset(root, commit, version, target, pinned), + /binary does not match/ + ); });🤖 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 `@npm/agentplugins/test/platform-proof.test.js` around lines 74 - 81, Add a test case around the existing frozenReleaseAsset assertions where the manifest and pinned metadata remain consistent, but the on-disk asset is replaced or written with different bytes; assert the byte-level mismatch error for the frozen release binary, thereby exercising the second branch after manifest validation succeeds..github/workflows/agentplugins-platform-proof.yml (1)
294-309: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueMove the expression expansions into
env:to satisfy zizmor.zizmor reports template injection on lines 295, 297, 302, 303, 307, and 308. The expanded values are not attacker-controlled today.
preparevalidatesinputs.expected_commitagainst^[0-9a-f]{40}$at line 144 and gates this job throughneeds: prepare, and theneeds.prepare.outputs.*values come from the validated tag and fromnpm pack --json.Binding the values through
env:removes the finding and keeps the step safe if the upstream validation is ever relaxed.♻️ Proposed change
- name: Run native npm launcher and released binary proof shell: bash + env: + EXPECTED_COMMIT: ${{ inputs.expected_commit }} + BOOTSTRAP_MODE: ${{ needs.prepare.outputs.bootstrap_mode }} + RELEASE_VERSION: ${{ needs.prepare.outputs.version }} + TARBALL_FILE: ${{ needs.prepare.outputs.tarball_file }} + PROOF_TARGET: ${{ matrix.target }} + PROOF_LIFECYCLE: ${{ matrix.lifecycle }} run: | - test "$(git rev-parse HEAD)" = "${{ inputs.expected_commit }}" + test "$(git rev-parse HEAD)" = "${EXPECTED_COMMIT}" release_assets="-" - if [[ "${{ needs.prepare.outputs.bootstrap_mode }}" = "local_frozen_asset" ]]; then + if [[ "${BOOTSTRAP_MODE}" = "local_frozen_asset" ]]; then release_assets="${{ runner.temp }}/agentplugins-proof-package/release-assets" test -d "${release_assets}" fi node npm/agentplugins/scripts/platform-proof.js \ - "${{ runner.temp }}/agentplugins-proof-package/${{ needs.prepare.outputs.tarball_file }}" \ - "${{ needs.prepare.outputs.version }}" \ - "${{ matrix.target }}" \ - "${{ matrix.lifecycle }}" \ - "${{ runner.temp }}/agentplugins-proof-${{ matrix.target }}.json" \ - "${{ inputs.expected_commit }}" \ - "${{ needs.prepare.outputs.bootstrap_mode }}" \ + "${{ runner.temp }}/agentplugins-proof-package/${TARBALL_FILE}" \ + "${RELEASE_VERSION}" \ + "${PROOF_TARGET}" \ + "${PROOF_LIFECYCLE}" \ + "${{ runner.temp }}/agentplugins-proof-${PROOF_TARGET}.json" \ + "${EXPECTED_COMMIT}" \ + "${BOOTSTRAP_MODE}" \ "${release_assets}"Note: the
Upload machine-readable native proofstep at line 315 must keep the same result path, somatrix.targetstays in thatpath:expression.🤖 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 @.github/workflows/agentplugins-platform-proof.yml around lines 294 - 309, Move the interpolated inputs used by the proof step’s shell script into the step-level env configuration, then reference those environment variables in the git check, bootstrap-mode branch, release-assets path, and platform-proof.js arguments. Preserve the existing values and behavior, while leaving matrix.target in the Upload machine-readable native proof step’s path expression unchanged.Source: Linters/SAST tools
🤖 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/planner/planner.go`:
- Around line 159-165: Update verify() to explicitly validate
CatalogCompatibility.Verification against the supported values, including
schema_only and not_tested; reject or map any unknown non-empty value to the
appropriate unverified/unsupported state before compatibility is accepted, while
preserving the existing warning and user-action behavior for supported values.
- Around line 90-97: Update the planner flow around the authentication checks
and plan status transitions so AuthenticationPending and
AuthenticationNotChecked produce PlanManualActivationRequired with
ActivationManual, rather than PlanReady or ActivationPrepared. Permit staging
and lifecycle completion only when authentication is AuthenticationNotRequired
or AuthenticationComplete; preserve the existing unsupported-plan handling and
user-action messages.
In `@npm/agentplugins/scripts/platform-proof.js`:
- Around line 146-148: Add AGENTPLUGINS_INTERNAL_PROOF_MODE and
AGENTPLUGINS_INTERNAL_PROOF_BINARY to the environment-variable removal list in
npm/agentplugins/scripts/platform-proof.js so the public_release_download branch
cannot inherit local overrides; preserve the local_frozen_asset branch’s later
assignment of these variables. No change is needed in
docs/agentplugins-release.md lines 94-95 because the existing wording remains
correct after this fix.
---
Nitpick comments:
In @.github/workflows/agentplugins-platform-proof.yml:
- Around line 294-309: Move the interpolated inputs used by the proof step’s
shell script into the step-level env configuration, then reference those
environment variables in the git check, bootstrap-mode branch, release-assets
path, and platform-proof.js arguments. Preserve the existing values and
behavior, while leaving matrix.target in the Upload machine-readable native
proof step’s path expression unchanged.
In `@npm/agentplugins/test/platform-proof.test.js`:
- Around line 74-81: Add a test case around the existing frozenReleaseAsset
assertions where the manifest and pinned metadata remain consistent, but the
on-disk asset is replaced or written with different bytes; assert the byte-level
mismatch error for the frozen release binary, thereby exercising the second
branch after manifest validation succeeds.
🪄 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: 222fb9d7-51e6-4cbc-a10a-da3fb8877204
📒 Files selected for processing (20)
.github/workflows/agentplugins-npm-publish.yml.github/workflows/agentplugins-platform-proof.yml.github/workflows/agentplugins-release.ymlcli/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/state_migration.godocs/agentplugins-release.mdinstall/integrationctl/agentplugins/adapters/clientdetect/detector.goinstall/integrationctl/agentplugins/adapters/clientdetect/detector_test.goinstall/integrationctl/agentplugins/planner/planner.goinstall/integrationctl/agentplugins/planner/planner_test.gonpm/agentplugins/bin/agentplugins.jsnpm/agentplugins/lib/bootstrap.jsnpm/agentplugins/scripts/platform-proof.jsnpm/agentplugins/test/bootstrap.test.jsnpm/agentplugins/test/platform-proof.test.jsrepotests/agentplugins_release_contract_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- install/integrationctl/agentplugins/planner/planner_test.go
- install/integrationctl/agentplugins/adapters/clientdetect/detector_test.go
- .github/workflows/agentplugins-release.yml
- install/integrationctl/agentplugins/adapters/clientdetect/detector.go
- cli/plugin-kit-ai/internal/agentpluginscli/add.go
- cli/plugin-kit-ai/internal/agentpluginscli/lifecycle.go
- .github/workflows/agentplugins-npm-publish.yml
| if plan.Status == domain.PlanUnsupported { | ||
| return plan, nil | ||
| } | ||
| if plan.Authentication == domain.AuthenticationPending { | ||
| plan.UserActions = append(plan.UserActions, "complete authentication for this plugin in the selected client") | ||
| } else if plan.Authentication == domain.AuthenticationNotChecked { | ||
| plan.UserActions = append(plan.UserActions, "verify the plugin's authentication requirements before using it") | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: all activation and completion paths gate on authentication state.
rg -n -C 8 \
'\b(AuthenticationPending|AuthenticationNotChecked|PlanReady|ActivationPrepared|DeliveryPlan)\b' \
--glob '*.go' .Repository: 777genius/plugin-kit-ai
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find delivery/lifecycle consumer files =="
git ls-files '*.go' | rg '(activator|transaction|kernel|stager|lifecycle|operation|install|complete|apply|activate|stage)' || true
echo
echo "== Search for ActivationPrepared and PlanReady gate conditions =="
python3 - <<'PY'
import subprocess, pathlib, re
files = subprocess.check_output(['git','ls-files','*.go'], text=True).splitlines()
for f in files:
txt = pathlib.Path(f).read_text()
hits = re.findall(r'\b(ActivationPrepared|PlanReady)\b', txt)
if hits:
line_map = {}
for m in re.finditer(r'\b(ActivationPrepared|PlanReady)\b', txt):
line = txt[:m.start()].count('\n') + 1
line_map.setdefault(line, []).append(m.group(1))
if len(line_map) <= 40:
print(f"\n--- {f} ---")
for line in sorted(line_map):
start=max(1,line-4); end=line+4
print(f"lines {start}-{end}:")
for i,s in enumerate(txt.splitlines()[start-1:end], start):
print(f"{i:4}: {s}")
PY
echo
echo "== Search exact consumer names around lifecycle/complete/stage/activate =="
rg -n -C 5 'Authentication(Pending|NotChecked|NotRequired|Complete|Failed)|Plan(Status|Ready|ManualActivationRequired|Unsupported)|Activation(Prepared|Active|Manual|Failed|NotRequired)|Complete|Lifecycle|Stage\(|Activate\(' \
install/integrationctl/agentplugins --glob '*.go' | head -n 300Repository: 777genius/plugin-kit-ai
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Planner relevant section =="
sed -n '70,105p' install/integrationctl/agentplugins/planner/planner.go
echo
echo "== Usecase service staging/bind path relevant section =="
sed -n '540,590p' install/integrationctl/agentplugins/usecase/service.go
echo
echo "== Provider Stager relevant section =="
sed -n '90,180p' install/integrationctl/agentplugins/providers/stager.go
echo
echo "== All functions calling DeliveryPlanner Plan / PackageStager Stage / kernel.ApplyDirectory =="
rg -n -C 4 '(\.Plan\(|\.Stage\(|ApplyDirectory\(|Activation:\s*state\.C|Activation:\s*plan\.Authentication|Authentication:\s*plan\.Authentication)' \
install/integrationctl/agentplugins --glob '*.go' | head -n 320Repository: 777genius/plugin-kit-ai
Length of output: 40523
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Activation readiness/use paths =="
rg -n -C 5 'initialLifecycle|ActivationPrepared|ActivationManual|ActivationActive|Authenticate|Authentication' \
install/integrationctl/agentplugins --glob '*.go' | head -n 260
echo
echo "== service.go around add flow =="
sed -n '100,280p' install/integrationctl/agentplugins/usecase/service.go
echo
echo "== service.go around initialLifecycle =="
rg -n -C 8 'func initialLifecycle' install/integrationctl/agentplugins --glob '*.go'Repository: 777genius/plugin-kit-ai
Length of output: 36280
Gate prepared activation behind satisfied authentication states.
PlanReady/ActivationPrepared are produced, staged, and committed even when plan.Authentication is AuthenticationPending or AuthenticationNotChecked. Change the planner to downgrade these cases to PlanManualActivationRequired/ActivationManual, and reject staging/lifecycle completion until authentication becomes AuthenticationNotRequired or AuthenticationComplete.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 93-93: QF1003: could use tagged switch on plan.Authentication
(staticcheck)
🤖 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/planner/planner.go` around lines 90 - 97,
Update the planner flow around the authentication checks and plan status
transitions so AuthenticationPending and AuthenticationNotChecked produce
PlanManualActivationRequired with ActivationManual, rather than PlanReady or
ActivationPrepared. Permit staging and lifecycle completion only when
authentication is AuthenticationNotRequired or AuthenticationComplete; preserve
the existing unsupported-plan handling and user-action messages.
| if compatibility.Verification == "schema_only" || compatibility.Verification == "not_tested" { | ||
| plan.Warnings = appendUnique(plan.Warnings, "catalog_"+compatibility.Verification) | ||
| if plan.Status != domain.PlanUnsupported { | ||
| plan.UserActions = append(plan.UserActions, "verify the plugin in the selected client before relying on it") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: the catalog schema or loader rejects unknown verification values.
rg -n -C 8 \
'CatalogCompatibility|Verification|schema_only|not_tested|tested' \
--glob '*.go' --glob '*.json' --glob '*.yaml' --glob '*.yml' .Repository: 777genius/plugin-kit-ai
Length of output: 50380
Reject unknown catalog verification values before accepting compatibility.
verify() only flags schema_only and not_tested; other non-empty values fall through without a warning or status change. Add explicit validation for CatalogCompatibility.Verification against the supported allowlist, or map unknown values to an unverified/unsupported state.
🤖 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/planner/planner.go` around lines 159 -
165, Update verify() to explicitly validate CatalogCompatibility.Verification
against the supported values, including schema_only and not_tested; reject or
map any unknown non-empty value to the appropriate unverified/unsupported state
before compatibility is accepted, while preserving the existing warning and
user-action behavior for supported values.
| for (const name of ["CODEX_HOME", "CLAUDE_CONFIG_DIR", "CURSOR_CONFIG_DIR", "NPM_TOKEN", "NODE_AUTH_TOKEN", "GITHUB_TOKEN", "GH_TOKEN"]) { | ||
| delete env[name]; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The public-release proof does not enforce the anonymous-download guarantee it documents. The runbook states that the public-release proof uses a cold cache without the local override, so it proves the normal anonymous GitHub release download. The proof script does not remove the internal proof variables from the inherited environment, so public_release_download mode can silently bootstrap from a local asset and still record anonymous_public_release_download: true.
npm/agentplugins/scripts/platform-proof.js#L146-L148: addAGENTPLUGINS_INTERNAL_PROOF_MODEandAGENTPLUGINS_INTERNAL_PROOF_BINARYto the removal list, so thepublic_release_downloadbranch cannot inherit a local override from the runner. Thelocal_frozen_assetbranch at line 172 sets them again after sanitation.docs/agentplugins-release.md#L94-L95: keep the wording once the script enforces the guarantee; no documentation change is needed after the fix.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 148-148: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(env.NPM_CONFIG_USERCONFIG, "registry=https://registry.npmjs.org/\n")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
📍 Affects 2 files
npm/agentplugins/scripts/platform-proof.js#L146-L148(this comment)docs/agentplugins-release.md#L94-L95
🤖 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 `@npm/agentplugins/scripts/platform-proof.js` around lines 146 - 148, Add
AGENTPLUGINS_INTERNAL_PROOF_MODE and AGENTPLUGINS_INTERNAL_PROOF_BINARY to the
environment-variable removal list in npm/agentplugins/scripts/platform-proof.js
so the public_release_download branch cannot inherit local overrides; preserve
the local_frozen_asset branch’s later assignment of these variables. No change
is needed in docs/agentplugins-release.md lines 94-95 because the existing
wording remains correct after this fix.
1 participant
What
Why
The existing Agent Plugins 1.0 package flow could materialize packages, but several manual activation, OAuth, verification, and release paths were stronger in documentation than in runtime evidence. This closes those launch blockers while preserving plugin.json as the standard-first install contract.
Safety
All agent/client behavior is covered through isolated fixtures and fake executables. No real user project, client configuration, OAuth session, or installed agent was mutated.
Validation
Summary by CodeRabbit
New Features
Release Improvements