refactor: installer core clean architecture refactor - #288
Merged
Merged
Conversation
…r core Full plan for bringing the agentplugins/agentpluginscli/cmd core up to parity with AgentBridge on SOLID/DRY/Clean Architecture/modularity/code quality (independent audit: 5.8/10 vs 7.4/10). Reviewed adversarially against the actual code before landing; six mandatory fixes applied.
|
Important Review skippedToo many files! This PR contains 434 files, which is 334 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (434)
You can disable this status message by setting the 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #288 +/- ##
==========================================
- Coverage 64.23% 59.95% -4.29%
==========================================
Files 1169 1321 +152
Lines 64109 67544 +3435
==========================================
- Hits 41182 40494 -688
- Misses 16949 21464 +4515
+ Partials 5978 5586 -392 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
* ci: add golangci-lint size and architecture gate Adds a single root .golangci.yml covering the three Go modules, wired into Required as a parallel matrix job. Two passes, because --new-from-* filters by changed line while funlen, gocyclo and revive file-length-limit report on the declaration line: growing an old function would otherwise slip through. - changed lines: correctness and style, only-new-issues - whole files: revive, funlen, gocyclo, gocognit, dupl, depguard depguard locks the two layering rules that already hold today: domain imports stdlib only, ports imports stdlib plus domain plus install/integrationctl/ports. That last one is a deliberate exception, documented in docs/ARCHITECTURE.md: Command and CommandResult are plain data types with no behavior, and duplicating them into domain would create a second source of truth. Files that already exceed the size limits are listed in a shrink-only legacy baseline (163 size entries plus 14 pre-existing revive style entries, generated by the first full run). scripts/check-lint-baseline.sh fails the build if the block grows. gocritic and gosec exclusions are picked from measured output, not guessed: six gocritic checks account for 699 of 799 findings and six gosec rules for 266 of 273. revive unused-parameter is off, it fires 172 times on interface implementations. Also fixes the gofmt drift in the seven files that had it, and adds make test-core, which runs the core suites with git hooks disabled so a user-level commit hook cannot fail unrelated tests. * ci: close the silent gaps in the lint gate Review found the changed-lines pass was a no-op for the root module, and the baseline was both too broad and too easy to widen. The root matrix leg no longer passes working-directory. The action runs the PR patch through path.relative(GITHUB_WORKSPACE, working-directory), which yields "" for "."; every `diff --git` section then fails the ` a//` test and is dropped, so --new-from-patch got an empty patch and the step passed on anything. Reproduced with an unchecked os.Setenv in cmd/agentplugins-registry-mirror: the unaltered patch reports errcheck, the rewritten one reports nothing. uniq-by-line is off. gocognit and gocyclo both report on the declaration line and only the first survived the default collapse, which hid 300 findings and would have made the baseline look complete while exempting a file from gocognit silently revealed its gocyclo finding on the next run. The baseline is regenerated per linter instead of per file. Two entries of 170 now carry all five size linters, down from 124; 62 carry three, 22 carry one. check-lint-baseline.sh fails on a base ref that does not resolve, instead of reporting success. It compares path, linters and message patterns rather than path alone, treats rules outside the markers the same way, and allows narrowing while rejecting widening. nolintlint now requires an explanation and a specific linter, which CONTRIBUTING already claimed. Adds pull-requests: read so the patch fetch cannot fall back to a full run, gofmt -s on the three remaining files, and corrects the gosec and gocritic counts now that findings are no longer collapsed.
) The legacy size baseline was generated on macOS. golangci-lint only analyses files whose build tags match the target, so not one file behind a linux or windows tag was ever looked at and none of them entered the block. CI lints on ubuntu, where `lint / install/integrationctl` goes red on adapters/process/osrunner_tree_linux.go with 21 findings, including a 1332 line file. Nobody is editing that file; it was simply never in the list. This did not surface on the Part 0a PR because lint.yml is only reachable through ci.yml, which triggered on pull_request for main and master, and Part 0a went into the refactor branch. core-fast.yml in the Part 0b PR is the first thing that actually ran it. The six entries come from re-running the size gate under GOOS=linux and GOOS=windows across the three lint modules, not from hand editing: cli/plugin-kit-ai/internal/authoring/scaffold/rename_windows.go cli/plugin-kit-ai/internal/promptio/cancel_windows.go install/integrationctl/adapters/process/osrunner_tree_linux.go install/integrationctl/adapters/process/osrunner_tree_windows.go install/integrationctl/agentplugins/adapters/packageview/scratch_windows.go install/integrationctl/agentplugins/adapters/packageview/source_windows.go packageview/source_windows.go at 682 lines is already named in the refactor plan as out of scope, which is how obvious the omission is in hindsight. check-lint-baseline.sh reports all six as new, by design. Its own header calls that out: the script catches accidental widening and pushes the deliberate kind through review. This is the deliberate kind, and it is the last time the block grows. The regeneration requirement is now written down in .golangci.yml above the block and in CONTRIBUTING.md, and the residual risk that the gate never sees windows and darwin files at all is recorded as item 13 in the plan's risk list.
#290) * test(agentplugins): add refactor guardrails for the install core (Part 0b) Behaviour is untouched. This adds the measurement layer the stacked installer-core refactor is checked against. archtest (install/integrationctl/agentplugins/internal/archtest) restates the depguard import rules as a plain go test, so the layering also holds without golangci-lint, and measures a per-package budget of client identity references. The baseline is generated, never hand written: cd install/integrationctl && go run ./agentplugins/internal/archtest -update The plan specified the metric as "domain selector matching ^Client[A-Z][A-Za-z0-9]*$". Taken literally that also counts domain.ClientID, ClientBinding, ClientSurface and ClientPackageRevision, which are type references rather than branching: the budget would grow whenever a signature gains a map[domain.ClientID], and no package could ever reach zero. The implementation therefore keeps the pattern as the first filter and then admits only the names domain declares as ClientID constants, read from the registry itself. The resulting numbers reproduce the independent count recorded in the plan (cli 89, usecase 40, clientdetect 22). Golden files freeze what the core produces today for all eleven clients: planning across four representative envelopes, the compatibility facade, detection on darwin/linux/windows, activation and deactivation with a fake command runner including VerifyOnly and unconfirmed previews, and the CLI --format json dry run. They render every exported field, including the operational paths the public json tags hide, and normalise volatile roots and identifiers so a rerun produces no diff. plan_detected_divergence.json records on purpose that cmd/agentplugins builds its planner with an empty Detected map while the CLI builds one with a real map, and that the two disagree about the native Copilot backend. The test fails if they ever start agreeing, so introducing PlanRequest cannot level this out by accident. add_normalize_target_test.go pins the --target vocabulary that domain.ParseClientID has to preserve: eleven ids, six aliases, case folding, and the lenient pass-through that turns an unknown name into a lowercased id rather than an error. CI: core-fast.yml is the merge gate for parts landing in the refactor branch (lint, test-core with coverage in the job summary, cross build for the three target platforms, go vet across all five workspace modules). vet-all is there because test-core does not compile the authoring commands, terminal prompts, conformance adapter, registry mirror or repotests, and all of them use core APIs the refactor changes. ci.yml gains a push trigger for refactor/** so the full lane runs after each merge instead of blocking every part for 10-27 minutes. * ci: fix two defects the first real core-fast run exposed `make test-core CORE_TEST_FLAGS=-cover` broke adapters/clientdetect. Its version probe re-executes the test binary and parses the child's stdout as JSON; under -cover the child appends "warning: GOCOVERDIR not set" to that stdout, because the probe deliberately strips the environment and cannot pass GOCOVERDIR down. The gate is back to a plain `make test-core`, identical to the local preflight, and coverage moved into a separate informational step that skips that one package. CORE_TEST_FLAGS is gone again with it. ci.yml failed at startup on the new refactor/** push trigger. The repository default workflow permission is contents:read, lint.yml asks for pull-requests:read so only-new-issues can read the PR diff, and a called workflow may not request more than its caller holds. The lint job now grants it explicitly. core-fast.yml already did, which is why its lint legs ran and ci.yml's did not; the job had never executed before, since ci.yml only triggered on main and master. * test(archtest): make the layer rule branch read as written deny and allow were selected through a nil check that was easy to misread. Verified by probe: a hujson import in domain, a planner import in ports and a providers import in usecase each fail their own rule. * docs(plan): correct the ClientID budget metric in section 7.2 The draft defined it as a regex on the selector name. That also counts domain.ClientID, ClientBinding, ClientSurface and ClientPackageRevision, which are type references, so the ratchet would fire on PlanRequest.Detected, Registry.Lookup and ParseClientID - changes the plan itself requires - and the DoD criterion "zero ClientID branching" would be unreachable, since usecase and ports must name that type. The metric now reads: names the domain package declares as ClientID constants, resolved by parsing domain itself. That reproduces the independent selector count already recorded in section 2.1, which is the evidence the regex was a drafting slip. Later parts reference the corrected definition. * ci: run the full lane on the refactor base branch only push: branches used the glob refactor/**, which also matches every part branch stacked on the base. The result on this branch was four full Required runs of 12-17 minutes, one per push, next to the core-fast run on the same commits - precisely the double spend section 10 of the plan set out to avoid by saying the full lane runs "after a part merges into the base branch". The base branch is now named explicitly. Section 10 of the plan is corrected in the same commit, in all three places that quoted the glob, and section 10's test-core row now matches what core-fast actually does: `make test-core` as the gate, coverage as a separate informational step, with the reason -cover cannot be a flag on the gate written down next to it. * test(providers): add the responding-client half of the lifecycle golden The golden set so far ran every client against a runner that answers with an empty result. Eight clients reach their normal outcome that way, but Claude and Kiro end in their unknown-evidence error and Cline stops at the missing ownership guard, so the happy path of the three most involved lifecycles was not pinned at all. Part 7 moves exactly this dispatch into per-client adapters and the plan rates it the riskiest part, so it needs both halves. Added, alongside the existing files rather than replacing them: activate_client_responds.json activate_client_responds_verify_only.json deactivate_recorded_ownership.json Claude gets its `plugin list --json` listing, Kiro an ACP session reporting the planned server connected (duplexLive, otherwise the reader hits EOF and Kiro reads that as the agent exiting early), and the removal case carries the native objects a completed Cline install would have recorded. All eleven clients now have a recorded happy path and a recorded silent-client path. archtest: a const spec with its own value ends type inheritance inside a block, so it can no longer be mistaken for a ClientID identity. No effect today, every const in domain/clients.go carries its type.
…#292) * refactor(agentplugins): route the install core through ports (Part 1) The use case layer no longer reaches for an adapter or the planner package. Path containment moves behind ports.PathPolicy and planning moves behind ports.DeliveryPlanner with a domain.PlanRequest, so both are injected instead of imported. - ports/paths.go: PathPolicy, implemented only by adapters/pathpolicy.Policy. internal/archtest fails the build on a second type carrying the whole method set, test files included, and ports/contracttest.RunPathPolicy is the mandatory harness: a permissive stand-in would silently drop the symlink and escape rejection that guards every destructive operation. - usecase.Service.Paths and planner.Planner.Paths are required with no default. A service or planner wired without one fails fast rather than running with weaker containment than the caller assumes. - ports.DeliveryPlanner.Plan takes domain.PlanRequest and applies the install intent itself, so a caller cannot forget the second step. Planner.Detected stays as the fallback when the request carries no surface map. - CommandRunner and its three opt-in capabilities move to ports/runner.go; providers keeps aliases. Command and CommandResult stay in the legacy ports package, as documented in docs/ARCHITECTURE.md. - The five use case type assertions become exported interfaces in ports/optional.go, with compile-time assertions in providers so a drifted signature breaks the build instead of quietly taking the fallback path. - clientVerifierAvailable moves to providers.Activator behind ports.ActivationVerifierClassifier. The client switch travels with it, which is why the archtest baseline shows usecase -9 and providers +9. - usecasetest.NewService and plannertest.NewPlanner wire the production policy in one place, so later parts do not repeat this edit across call sites. Behaviour is unchanged: the Part 0b golden files are byte identical. * chore(authoring-docs): advance the source pins Part 1 moved The docs exporter pins a sha256 for every non-test Go file in the construction packages, and the inventory check rejects one it does not know. Part 1 edited three pinned CLI files and added domain/planning.go, so the pins advance with them, the same way dev_session.go advanced in b443eb7. * fix(agentplugins): close two ways around the PathPolicy gate Both were found by running real exploits against the gate, not by reading it. The archtest rule only reported a type that declared the whole method set, so `struct{ pathpolicy.Policy }` with a single overriding RequireExactPath walked through it: embedding supplies the other two methods, the type satisfies the port, a Service accepts it, and the os.Lstat symlink rejection is gone. One declared contract method is now enough to report. A type that embeds and overrides nothing still is not reported, because it behaves exactly like the real Policy. The same change closes the alias variant, since the shape check no longer reads parameter and result type names: `type s = string` is string. The contract harness built its escape case with filepath.Join, which cleans, so the ".." never reached the implementation and the case only asserted "a path outside base is rejected". A naive policy comparing raw strings with HasPrefix passed the whole harness. The candidate is now concatenated, and two cases join it: an unnormalized spelling of the exact managed path has to be accepted, so the harness catches an implementation that is too strict as well as one that is too permissive, and a relative candidate to RequireContainedChild has to be rejected. The symlink cases move into named subtests. A host that refuses os.Symlink now skips visibly instead of logging and continuing, which had made the two strongest negative assertions disappear into a passing run. docs/ARCHITECTURE.md listed imports for usecase that left out install/integrationctl/ports. The use case does import it, for LockManager, and that is not the data-only exception the document already records, so it is written down as its own.
…Part 2) (#293) * refactor(agentplugins): add the clients contract and shared helpers (Part 2) Establishes the extension point the later parts migrate onto, and pays the DRY debt before any client code moves, so Part 5 relocates deduplicated files instead of duplicated ones. No behavior changes: the planner, detection and activation golden files are byte-identical. New `agentplugins/clients` holds the segregated capability contract (Adapter, HostDetector, TargetLayout, PlanRefiner, CompatibilityLimiter, StagingLayout, Projector, Lifecycle, ActivationPreflighter, AutomaticActivator, ReadOnlyVerifier, RegistryInspector, PreparedRegistryInspector, SelectionReader, Env, RegistryFinding) plus the Registry that resolves a client id to its adapter. NewRegistry rejects a duplicate id and an id domain does not define; All follows domain.ClientDefinitions order; As[T] is the capability lookup. A nil Registry stays inert and the dispatchers built on it will fail with ErrRegistryRequired - resolving nil to "every client" would compile every adapter into any binary importing a generic package and take the registry out of the composition root's hands. `clients/contracttest` carries the harness that compensates for As[T] being a runtime assertion: RunAdapter today, and RunTraitParity, which checks a declared client trait against the interface the declaration promises. The traits land in Part 9, so it ships running on an empty requirement set; the mechanism itself is covered by tests through the pure violations functions each Run* reports. `providers/nativeconfig` moves to `adapters/nativeconfig`. It is a generic content-addressed kernel with no dependency on providers, and the contract needs it for clients.Env. It also pulls hujson and atomicfile, which is why ARCHITECTURE.md now records "close to extractable, one known exception" instead of claiming the contract is a leaf package. `clients/shared` collapses the duplicates the audit found: - six textually identical "only skills and MCP servers" predicates (planner.hasOnlyKiroNativeComponents, hasOnlyPortableNativeComponents, geminiNativePlanComponents, providers.kiroNativeComponents, openCodeNativeComponents, geminiNativeComponents) -> OnlyNativeComponents - three component-kind predicates (providers.componentKindPresent, stager.hasSupported, planner.hasSupportedKind) -> ComponentKindPresent - three copyString manifest builders (projectOpenAI, projectCursor, projectedOpenAIManifest) -> ManifestFromEnvelope with WithAuthorObject/WithAuthorNameEmail - three MCP projection loops (projectOpenAIMCP, projectCursorMCP, projectKiroMCP) -> ProjectMCPServers with the openai/cursor/kiro dialects - five ready-promotion blocks in the planner -> PromoteNativeReady - three identical native-config activation branches (Cline, OpenCode, Gemini) -> one nativeConfigActivation shape in the activator Every unexported helper the client-specific files reach for moves with them, exported: JSON decoding and strict objects, the stdio data contract, path resolution, marketplace naming, registry findings, the bounded native diagnostic, capacity checks, and the platform-tagged exclusive rename. Leaving them behind would have closed a providers <-> clients/<id> import cycle in Part 5. providers.ManagedMarketplaceName stays exported as a thin wrapper: the CLI and the root-module repotests use it. Two depguard rules lock the direction: clients-no-upward stops an adapter from importing providers, planner, usecase or clientdetect, and clients-no-concrete-clients stops the contract, its shared helpers and any client package from importing another client or the assembled registry. Both are declared for the packages Parts 3-5 create, and both were verified to fire. A git mv of an exempt file looks the same as a new exemption to the shrink-only baseline check, so moves are now declared in scripts/lint-baseline-renames.txt. The rename only maps the path; the entry still has to carry the same linters and message patterns. * fix(lint): verify a declared baseline rename against git The rename mechanism compared the entry under the new path against the one the old path carried, and trusted the declaration for everything else. That left the file as a way to buy amnesty for new code, which is the exact thing the shrink-only gate exists to stop. Two bypasses were reproducible: - repoint an existing exemption at an untouched file and declare the pair, and the run passes; - point two exemptions at one new path, and the per-path union in compare_rules reads the widened result as unchanged. A declaration now has to describe a move git can confirm: the old path was exempt at the base revision and is gone from the working tree, the new path exists and was not at the base, no two lines name the same file, and `git diff -M` reports the pair as a rename. A move that also rewrites the file past the similarity threshold is a rewrite, and a rewritten file does not keep its amnesty. The patterns are reduced to literal paths by undoing the anchors and the `\.` escapes the generator emits; a pattern that is not one literal file is rejected rather than guessed at. Lines with the wrong field count are named instead of silently skipped, and a line whose move has already landed in the base is reported as removable rather than failing, so a leftover cannot block the next change. Also stop contracttest.RunTraitParity from passing on a nil registry. All() on a nil receiver yields nothing, so the check ran over zero adapters and went green - which is what an unchecked error from NewRegistry leaves behind, and exactly when the parity test is supposed to speak up.
#294) * refactor(agentplugins): add per-client detection adapters and the default registry Moves the eleven detectX bodies out of the generic detector and into one package per client, each implementing clients.Adapter and clients.HostDetector. The bodies are transferred as they were: the only change is that a client reaches the machine through clients.Host instead of through the detector's own fields. The detection golden files are byte-identical. The surface constructors move into the contract rather than into each client package, because the evidence strings they produce (executable_on_path, configuration_directory, application_bundle, application_installation, desktop_entry, editor_extension) are a cross-client output contract. clients.NewHost builds that implementation from HostProbes, so the detector and the contract harness share one copy. Host also gains ResolvedBinarySurface: Kiro and Windsurf resolve a binary once and use the path both as evidence and as the executable, and without it they would have to probe PATH twice or restate the evidence string themselves. clients/all is the single place that names every adapter, which makes it the only package that links all of them into a binary. Default builds the registry on first use and panics on a duplicate or unknown id: that is a mistake in the list, not a runtime condition, and every caller would otherwise translate the same error into the same crash. contracttest.RunHostDetector covers the structural half of detection on all three operating systems: surface ids are unique and non-empty, an undetected surface carries no evidence, a selection surface is one of the reported ones, and the observation is identical across two runs including the number of probe calls. The probes count LookPath, Lstat and ReadDir, so an adapter that read the real machine would show up as non-deterministic. Its own tests drive the pure violations function with a deliberately broken adapter, so the harness is shown to reject as well as to accept. * refactor(agentplugins): route client detection through the registry clientdetect.Detector loses its switch over ClientID: detect iterates the injected registry, asks each adapter for the surfaces it observed, and keeps the generic half - detection status from the selection surfaces, display name from domain.ClientDefinitions, the version probe and the stable ordering by client id. The archtest budget for the package drops from 22 client identity references to zero and it leaves the lint size baseline; the detection golden files for darwin, linux and windows are unchanged. Registry is required rather than defaulted. NewOS keeps its signature and leaves the field unset, so cmd/agentplugins assigns clients/all.Default and stays the one place that decides which clients a binary knows about. Defaulting it in the constructor would have linked every adapter into any binary that merely detects, which is the service locator the contract exists to avoid; the new depguard rule and the matching archtest boundary keep providers, planner and clientdetect from importing clients/all outside their tests. The version probe moves into its own file next to the detector. Two findings the linter reports on the moved lines are fixed there rather than carried over: the deferred cleanup ignores its error explicitly, and the trailing-dot trim drops the redundant HasSuffix guard. clients/all also carries the parity test between the declarative registry and the adapters: every domain client has exactly one adapter and every adapter has a definition, and every adapter is run through contracttest.RunHostDetector. * test(agentplugins): make the detection harness match what it claims RunHostDetector said it proved the adapter reaches the machine only through clients.Host, but the probe counters cannot see past the host: an adapter calling os.Stat directly left them untouched and passed. The harness now also requires that nothing comes back detected and that no executable is reported, since the host answered "does not exist" to every question - an ambient read that reaches the Detection shows up there. That is a necessary condition rather than a sandbox, and the doc comment, the test names and ARCHITECTURE.md now say so instead of claiming a guarantee. One repeat only coin-flipped on a map-backed adapter, so an observation is now taken eight times and each run is compared against the first, in content, in surface order and in probe counts. Also records in the plan where the shared Copilot/VSCode logic goes. They are separate packages with one backend family, fourteen non-test references to ClientVSCode in providers and planner are shared branches, and clients-no-concrete-clients forbids an import between the two packages - so clients/shared is the only legal home, and Parts 5, 7a and 8 now name it instead of clients/copilot. Two smaller corrections: the layer table prose still claimed the adapter row had no rule, and SelectionSurfaceIDs was documented with Windsurf channels as its example although Claude is its only user.
777genius
added this pull request to stack #297
September 18, 2026 08:11
* docs(plan): finalize Part 12 contract module plan after review Rewrites the Part 12 draft with the 13 review corrections: the nativeconfig DTO-split churn is 193 lines (neutralized by a 22-line alias block in the implementation package), providers.Activator.NativeConfig and its five construction sites are in scope, the clients depguard rules are deny lists so Part 12a adds a rule instead of removing an allow, out-of-tree fitness is proven by GOWORK=off rather than an in-repo import, and five CI surfaces that would silently stop seeing the contract get a concrete workflow edit each. Adds a spec for the guard test to land now, before Part 5, and records a finding neither the draft nor the review had: domain's external test package imports adapters/pathpolicy, a third in-monorepo edge that would pull golang.org/x/text and a module cycle into the new module's require block. Revises the forecast down: modularity 7.5-8 with an explicit caveat that clients/shared stays in the monolith, plan reliability and confidence 7/10. * test(agentplugins): guard the contract layer's dependency closure domain, ports and clients are the three packages Part 12 will extract into their own Go module. TestContractLayerDependencyClosure fixes today's three internal-repo edges leaving that layer (ports -> install/integrationctl/ports, clients -> adapters/nativeconfig, and domain's identity_portable_test.go -> adapters/pathpolicy) plus the full transitive closure and external module set they pull in, so a new dependency introduced anywhere in Part 5-9 fails on the introducing PR instead of surfacing at the start of Part 12. See docs/plans/installer-core-part-12-contract-module.md §12.1.G for the guard test's spec and §12.1(c) for the dependency snapshot it enforces. * test(agentplugins): sweep contract-layer deps across GOOS The guard test only saw the host platform, so a new Windows-only external module in nativeconfig would pass on macOS/Linux CI. Recompute the closure for linux, darwin and windows and record golang.org/x/sys in the baseline. * docs(plan): attribute the Windows x/sys edge to nativeconfig only The guard-test review found the plan still listed lock_windows.go as stdlib-only and blamed atomicfile for hiding golang.org/x/sys. Only the nativeconfig Windows files import that module; host-platform CI is the gap, not go list itself.
…#304) * refactor(agentplugins): planning through the client registry (Part 4) planner.Plan loses its switches over ClientID. The pipeline is fixed and generic now - start from the declarative capabilities, apply the two rejections the registry alone can decide, resolve the target, decide the components, apply the catalog verdict and the package diagnostics, then let the client speak. The archtest budget for the package drops from 44 client identity references to zero, planner.go leaves the lint size baseline, and every golden plan and compatibility file is byte-identical, including plan_detected_divergence.json. Seven named planning stages rather than one refiner. A single RefinePlan could not carry the existing behaviour: the order of the warnings and user actions a plan renders is part of the observable contract, and the client-specific decisions do not all happen at the same point. So each stage is named for where it runs. NativeRegistryLayout: VS Code records the sibling Copilot registry instead of its own. PlanPrecondition: Claude refuses to plan without its CLI, before a target is resolved, so the rejection never carries resolved paths. TargetLayout: Claude and Cursor deliver into the directory the client already reads. LocalPreparation- Authorizer: ChatGPT accepts a personal Context7 receipt in place of pinned catalog evidence. PlanQualifier: ChatGPT rejects a package whose MCP servers no registered app binding covers, ahead of the generic "no usable components" verdict. PlanRefiner: readiness promotions, user actions, warnings. PreparationRefiner: the prepare install intent, kept separate because ApplyInstallIntent is also applied on its own to a plan that was already built. A client implements only the stages it speaks in. Three branches become declarative instead of moving into an adapter, because the planner needs them before it has anything to dispatch to. "ChatGPT plans even when it is not installed here" is ClientDefinition.PlansWithoutHostPresence. "An app diagnostic destroys a component" is capabilities.AppSupport != SupportUnsupported, which is ChatGPT and only ChatGPT today. "A declared sse server the client cannot take gets a named reason rather than the generic one" is now any client that does take some other MCP transport, which is Codex and OpenCode today, and stays the generic reason for a client that takes no MCP at all. Each is equivalent on the current registry - the golden files pin that - and each stays correct as the registry grows. domain/directory_context7_preparation.go, the branch the plan added to this part as edit F-2, is resolved by declaring the relationship rather than by adding a boolean trait. ClientDefinition gains DirectoryPreparationPurpose: the single bounded resolve purpose a client may acquire source under, empty for every client but ChatGPT. The validation asks whether the target client declared the purpose the request carries, so an unknown purpose matches no client and is rejected for that reason instead of by name. A trait flag would have said "this client is allowed a preparation purpose" without saying which, leaving the purpose constant in domain anyway. Copilot and VS Code share the promotion rule through clients/shared, as Part 3 recorded: PromoteBackendReady marks a plan ready when the CLI that installs on the client's behalf was found, and the two adapters differ only in which executable they resolve for it - its own for Copilot, the backend sibling's for VS Code. domain.BackendSiblings replaces the hardcoded Copilot/VS Code pair in DetectedPhysicalClient and in PlanInput.BackendSibling. The planner facade keeps its names for callers outside the core (Capabilities, Compatibility, ClientCompatibility, ApplyInstallIntent, DetectedPhysicalClient, KiroPrepareAction, ChatGPTAppBindingAction; the last two now re-exported from the adapters that own the text). Compatibility and ApplyInstallIntent take the registry as their first argument, because they are package-level and there is no planner value to read one from; defaulting it would have linked every adapter into any binary that merely reports compatibility. cli/internal/authoring is its own entry point with no composition root above it, so it names the full set there. contracttest.RunPlanRefiner runs against every adapter in clients/all that implements PlanRefiner, on a viable plan and on an unsupported one: the plan identity comes back unchanged, an unsupported plan is never promoted, and refining twice equals refining once. Verified by breaking a refiner deliberately - all three violation classes are reported. Two findings on the new lines are fixed rather than carried: the pipeline returned `plan, refiner.RefinePlan(ctx, input, &plan)`, where Go does not specify that the mutation is visible in the returned copy, and three moved error strings that now start a sentence lead with an article, the form claude_probe.go already uses. * fix(authoring): hand the installer fixtures a client registry The three authoring fixtures that drive the real add command through agentpluginscli.App build the planner from App.ClientRegistry, which this part made required, so they failed with "client registry is required" before reaching any assertion: TestGeneratedPackagesReachExistingInstaller- Planner, TestPackedInstallerSourceHarness and TestPackedInstallerAssessmentBoundaries. They are skipped on macOS - writable native authoring needs Linux or Windows - so the local run showed nothing and only the Authoring workflow caught it. The other App literals in the tree construct the command tree for discovery and never plan, which is why they have always left the installer dependencies unset. * docs(plan): warn Part 7c off depending on the concrete nativeconfig.Kernel type Part 12 (planned after Part 11) will replace this with a narrow ports interface. Flagging it now, while the constraint costs nothing, avoids a rewrite later. * refactor(agentplugins): inject the client registry into authoring too readiness.Compatibility named clients/all itself, which linked all eleven adapters into plugin-kit-ai: the authoring binary went from 2 client packages to 14 and grew ~78 KB. That is the service locator edit A-3 forbids, and the justification given for it - "authoring has no composition root above it" - was simply wrong. commands.App is one, and it is assembled in three mains. commands.App now carries the registry and hands it down; the three mains name the set. The dependency count does not fall back to 2 and cannot: the ChatGPT and Windsurf limitations a compat report prints moved into the adapters in this part, so a binary that prints them has to link them. What changes is who decides - a main rather than a library - and the cost is now visible where it is paid. depguard rule dispatchers-take-an-injected-registry becomes libraries-take-an-injected-registry and covers cli/plugin-kit-ai/internal as well, mirrored by a new archtest boundary. Both were checked against a deliberately reintroduced import. App.ClientRegistry stays a runtime invariant, so TestPlanningFailsClosedWithoutAClientRegistry drives add through an App without one and asserts the error names the missing registry rather than a panic, and checks the doctor path degrades instead of crashing. It is the local version of the check that previously only the Authoring workflow could make. The authoring fixtures that run compat get a registry for the same reason. Also from review: - contracttest.RunPlanRefiner runs unconditionally per adapter. The "if it implements PlanRefiner" guard turned a renamed method into a silent skip, which is the failure the harness exists to catch. - domain pins that every declared DirectoryPreparationPurpose is a known constant, so a typo cannot quietly un-declare a client. - Three branches that no test reached: Claude without its CLI (rejected before a target is resolved), Copilot without its CLI (not promoted), and the personal ChatGPT mapping, accepted and rejected both ways. - The plan no longer claims the planner facade signatures are unchanged, and no longer asks for a domain archtest budget the metric cannot produce. The three reworded error strings are recorded as a named exception instead of an unmentioned one.
…ter (Part 5 probe) (#305) * refactor(agentplugins): move Kiro native and ACP into the client adapter Part 5 starts with Kiro as the probe: filesystem mutation, ACP verification, and pipe evidence live in clients/kiro, split under the file-length gate. Skill digest and object-map helpers move to clients/shared so Gemini, OpenCode, and Cline do not import Kiro. Providers call the exported Kiro surface until Part 8 turns on generic-no-concrete. * fix(agentplugins): split the Kiro adapter so the install-core lint gate can land The moved files still tripped the whole-file size gate, and new-file changed-line lint treated every product-name error as ST1005. Extract the units and keep the golden-pinned wording without wrapping the original causes.
…ckages (Part 5 remainder) (#307) * refactor(agentplugins): move remaining native adapters into client packages Finish the Part 5 file move with 1:1 git mv so size-gate exemptions travel with the files. Keep Activator/Stager tests in providers so client packages do not import upward. * fix(lint): close the changed-line findings on the Part 5 remainder move Group the Codex test imports, give the exported Claude probe errors a leading article, and switch Cline object-kind validation so QF1003 is quiet without splitting the native files.
…ayout (Part 6) (#308) Stager loses its ClientID switch. The generic pipeline copies, sanitizes, and digests; each client adapter projects into the sanitized tree and returns the objects it owns. ChatGPT writes .app.json itself. Claude keeps staging beside the watched skills root via StagingLayout.
… (Part 7a) (#309) * refactor(agentplugins): route CLI-registry lifecycle through adapters (Part 7a) * fix(agentplugins): close Part 7a lint and fail-closed verifier DIP New adapter paths must wrap both fallback errors, drop unused helpers, and treat a missing registry as no verifier so lifecycle dispatch stays fail-closed.
…(Part 7b) (#310) * refactor(agentplugins): route Kiro ACP lifecycle through the adapter (Part 7b) Move prepare, duplex preflight, native mutation and ACP observation into the Kiro adapter so Activator dispatches by registry instead of a ClientKiro switch. * fix(agentplugins): share the registry-required sentinel on Activator Dispatchers should fail closed with the same clients.ErrRegistryRequired value, including the boolean lifecycle predicates a leftover native client would otherwise claim.
…s (Part 7c) (#311) Move Cline, Gemini, OpenCode and Windsurf activate/deactivate onto registry adapters and drop the leftover ClientID switch from Activator. Native-config clients use Env.NativeConfig by value through Apply/ApplyBatch.
…ters (Part 8) (#312) Move RegistryInspector/PreparedRegistryInspector into client packages and enable generic-no-concrete so providers and planner no longer import adapters.
…ientTraits (Part 9a) (#313) * refactor(agentplugins): replace ClientID switches with declarative ClientTraits (Part 9a) Installer policy now lives on the client table, so usecase and domain stop naming Codex, Claude, Copilot, or ChatGPT and a new client is a row plus contracttest parity rather than another switch. * fix(agentplugins): wrap both repair verify errors and drop stale trait-parity comments errorlint rejects %v on the new DisplayName format string, and the Grok nits were leftover Part 9 wording plus Allows claiming Validate's automatic exception.
…phase methods (Part 9b) (#314) Extract-method the four remaining usecase monoliths so service.go, group.go, repair.go, and remove_group.go can leave the size baseline without changing call order or group goldens.
…position (Part 10) (#315) * refactor(agentplugins): inject planner and client traits into CLI composition (Part 10) Move ClientID aliases and CLI client policy behind domain traits and the composition-root Planner/Targets so commands stop reconstructing core internals. * fix(cli): close new-file lint on the Part 10 add extract The extracted add helpers left the size baseline, so errcheck, errorlint, gocritic, goimports, and unconvert now apply to those lines.
5 tasks
…ansitional aliases (Part 11) (#316) Detection is request-scoped, generic packages no longer name clients, and in-scope files leave the size baseline so adding a client stays three edits plus contracttest.
* refactor(agentplugins): extract the install core into a nested Go module Give agentplugins its own go.mod so GOWORK=off CI, coverage, and vuln scans see the tree; keep import paths and a one-way replace to the parent. * fix(agentplugins): point leftover parent-relative tests at the nested module Keep the release-contract ratchet on the nested go test/vet lines so dropping the module from Makefile cannot stay green.
#318) * fix(agentplugins): attest the nested module in authoring-docs factory pins Required and Coverage failed because go.work now names a module the docs fixture did not copy. Keep the v0.1.65 overlay attesting that tag's bytes. * docs(agentplugins): name current-only files the v0.1.65 overlay drops The ENOENT path also drops domain/planning.go, not only the nested module go.mod.
6 tasks
workflow_call hides pull_request from golangci-lint-action only-new-issues, so Required on the architecture PR linted every module. Match make lint and fix the ST1005/unused findings that are actually new versus main.
…ify, and identity (#320) * docs(agentplugins): record both composition roots and the merge-base lint gate ADR 0007 and ARCHITECTURE claimed cmd/agentplugins was the only production clients/all importer; authoring's plugin-kit-ai binary is a second root. Coverage now cds into the nested module, and the plan matches the lint.yml that #319 actually shipped. * fix(agentplugins): thread one nativeconfig kernel through activate, verify, and identity inspect Activate wrote through env.NativeConfig while Verify and registry inspect constructed nativeconfig.New(), so an injected kernel never observed its own writes. * fix(agentplugins): fail closed on a missing nativeconfig kernel An omitted NativeConfig used to construct a second OS kernel, so production and tests could not tell a wired kernel from a default. Zero Kernel now fails Inspect/Apply; Kiro re-runs duplex preflight before native writes; Makefile/core-fast cd into the nested module under GOWORK=off. * fix(agentplugins): fail closed when a native client has no projector Staging a native or native-config client without As[Projector] used to succeed with empty native objects. Claude and ChatGPT Activate now run PreflightActivation themselves so a skipped dispatcher lookup cannot bypass the probe. * fix(agentplugins): refuse skill-only native writes without a kernel A zero nativeconfig kernel skipped ApplyBatch on Cline/OpenCode skill-only paths, so omitted NativeConfig still mutated the host tree. Require FileIO before those writes and keep GOWORK=off test-core inside nested modules. * fix(agentplugins): refuse Gemini skill writes without a kernel Gemini installs skills before ApplyBatch, so a zero kernel still mutated the host tree and only failed on the later MCP batch. Require FileIO on the Gemini and Windsurf native entry points before any skill or config IO. * fix(agentplugins): inspect Cline and OpenCode native identity before commit Always-Clear InspectNativeRegistry let colliding skills and MCP names pass the pre-commit gate. Mirror Gemini: empty registry root is indeterminate, and a present host name is a collision. * fix(agentplugins): allow missing OpenCode objects during identity inspect Repair must observe identity after a host wiped managed MCP entries. Exact verify stays on Activate; identity inspect now matches Gemini's allow-missing check. * fix(agentplugins): allow missing Windsurf MCP during identity inspect Wiping a managed Windsurf entry must still be repairable. Identity now uses allow-missing verify like Gemini; Activate still requires exact ownership. * test(agentplugins): cover Windsurf RepairGroup after wiped native MCP Single-client Repair skips identity inspect. RepairGroup and Update are the production gates that must restore a missing owned Windsurf entry. * fix(agentplugins): inspect OpenCode and Windsurf host identity without the package RepairGroup refused a missing managed directory because inspect still verified the live ActivePath projection. Host-only inspect matches Cline and Gemini so a wiped package can be restaged.
2 tasks
… commit Identity inspect now classifies a foreign host MCP key as unmanaged before UpdateGroup or RepairGroup mutate managed bookkeeping. The hosted OpenCode lifecycle still expected ActivationFailed after commit.
5 tasks
… repair Update now refuses a foreign host MCP key before commit, so the fixture 3.0.0 tree is not the applied revision. Restore the managed package so repair can observe that collision.
2 tasks
…repair Copying the managed package directory left repair preflight comparing the published artifact to the recorded source envelope. Restore the fixture snapshot from before the 3.0.0 probe.
3 tasks
Root module is go 1.22, so stdversion rejects os.CopyFS. Snapshot and restore the applied source with filepath.Walk.
777genius
marked this pull request as ready for review
September 19, 2026 09:27
777genius
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tracking PR for bringing the installer core (
install/integrationctl/agentplugins,cli/plugin-kit-ai/internal/agentpluginscli,cli/plugin-kit-ai/cmd/agentplugins) up to parity with other project on SOLID, DRY, Clean Architecture, modularity, and code quality. An independent audit scored our core 5.8/10 against their 7.4/10 on these criteria.This is a work-in-progress base branch. Individual parts land here as separate PRs (lint gate, ports/DIP, client adapter contract, per-client migration, CI guardrails, etc). This PR itself will not be merged into
mainuntil the whole refactor is done and explicitly approved.Full plan (target architecture, part-by-part breakdown, lint gate config, CI mechanics, adversarial review results):
docs/plans/installer-core-clean-architecture-plan.md.Test plan
mainonly on explicit approval