refactor(adapters): cut ~3,100 LOC of redundancy in the agent adapter layer (#2349)#2355
Open
harshitsinghbhandari wants to merge 8 commits into
Conversation
…r#2349) Introduce the shared building blocks the per-adapter cleanup will use: - ports.NormalizePermissionMode (finding 3) - hookutil.FileExists (finding 10) - binaryutil.ResolveBinary + BinarySpec (finding 2) - activitystate.StandardDeriveActivityState (finding 4) - agentbase.Base embed + StandardSessionInfo (findings 6-9) No adapters wired up yet; behavior unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…Wrapper#2349) Reference conversion proving the shared packages against real tests: - hooks.go collapses onto hooksjson.Manager (finding 1) - ResolveGooseBinary via binaryutil.BinarySpec (finding 2) - gooseMode uses ports.NormalizePermissionMode (finding 3) - activity.go deleted; dispatch points at activitystate (findings 4, 5) - SessionInfo via agentbase.StandardSessionInfo; Base embed drops the GetConfigSpec/GetPromptDeliveryStrategy no-ops (findings 6-8) - fileExists/atomicWriteFile copies removed (findings 5, 10) Also adds hooksjson package + activitystate test. goose: 327->~90 LOC. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pers (AgentWrapper#2349) Applies the shared helpers across every remaining adapter: - hooksjson.Manager for the matcher-group cohort (claudecode, qwen, droid) - binaryutil.BinarySpec for 19 binary resolvers (aider/cursor/opencode/codex keep their special resolvers; all now use hookutil.FileExists) - ports.NormalizePermissionMode replaces 15 private copies - agentbase.Base embed drops the GetConfigSpec/GetPromptDeliveryStrategy no-ops (and GetAgentHooks/GetRestoreCommand/SessionInfo no-ops on hookless adapters) - agentbase.StandardSessionInfo replaces the per-adapter metadata readers - activitystate.StandardDeriveActivityState via dispatch for the 8 name-only derivers; their activity.go files removed (claudecode/codex/droid/agy/opencode keep payload-parsing derivers) - 4 private atomicWriteFile copies missing fsync now use hookutil.AtomicWriteFile - 23 private fileExists copies removed Full backend build + vet + test suite green (1647 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Vaibhaav-Tiwari
requested changes
Jul 3, 2026
Vaibhaav-Tiwari
left a comment
Vaibhaav-Tiwari
left a comment
Collaborator
There was a problem hiding this comment.
Kiro binary lookup changes on Windows:
Before this PR, Kiro looked here first:
%LOCALAPPDATA%\Programs\kiro\kiro-cli.exe
After this PR, the shared binary resolver checks this first:
%APPDATA%\npm\kiro-cli.*
So if a Windows user has both installed, AO may now launch the npm shim instead of the native Kiro install. That is a behavior change and should be fixed.
…rapper#2349) Review feedback: the shared resolver hardcoded Windows candidate order as APPDATA then LOCALAPPDATA, which flipped Kiro's lookup so the npm shim (%APPDATA%\npm\kiro-cli.*) was probed before the native install (%LOCALAPPDATA%\Programs\kiro\kiro-cli.exe). A fixed order can't preserve every adapter's original order (goose/vibe want APPDATA first, kiro wants LOCALAPPDATA first), so BinarySpec now takes an ordered WinPaths []WinPath list where each entry names its base (WinAppData/WinLocalAppData/WinHome). Every adapter's list reproduces its pre-refactor order exactly; kiro's native path is restored to first. go build / vet / test all green (1647 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
Good catch, thanks @Vaibhaav-Tiwari. Fixed in 9b1fc51. Root cause: the shared resolver hardcoded Windows candidate order as |
…or-18/adapter-dedup
…gentWrapper#2349) Embedding agentbase.Base (value receiver) lets govet's unusedwrite analyzer prove that setting resolvedBinary in tests that only call Base-promoted methods (GetConfigSpec/GetPromptDeliveryStrategy/SessionInfo/cancellation) is a dead write. Drop the field from those 23 constructions; GetLaunchCommand/GetRestore tests that actually read it keep it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Inherited via the merge of main: a SessionRecord literal aligned its Metadata field across a multi-key line, which gofmt/goimports rejects. One-line reformat to unblock CI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Vaibhaav-Tiwari
approved these changes
Jul 3, 2026
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2349.
Consolidates the copy-pasted logic across the 23 agent adapters in
backend/internal/adapters/agent/into a handful of shared helpers, with no behavioral change and one latent correctness fix (missingfsync).Net delta
-3,105 LOC of non-test production code (-3,454 total incl. tests): 78 files changed, 1,212 insertions, 4,666 deletions.
New shared packages
hooksjson— generic matcher-group hooks file (Read/Write/Install/Uninstall/AreInstalled). The claude/goose/qwen/droid hooks.go each collapse from ~180-380 LOC to ~60.binaryutil.ResolveBinary(ctx, BinarySpec)— table-driven binary search; per-adapter resolvers shrink to a ~10-lineBinarySpecliteral.agentbase.Base(embed) +agentbase.StandardSessionInfo— default no-opports.Agentmethods + the shared session-metadata reader.activitystate.StandardDeriveActivityState— the name-only hook→activity mapping shared viaactivitydispatch.ports.NormalizePermissionModeandhookutil.FileExists— the two most-copied one-liners.What was removed (findings from #2349)
hooksjson.Manager(claudecode, goose, qwen, droid)binaryutil.BinarySpec(19 resolvers)normalizePermissionMode×15ports.NormalizePermissionModeDeriveActivityState×8activitystate.StandardDeriveActivityState; activity.go deletedatomicWriteFile×7 (4 missing fsync)hookutil.AtomicWriteFileSessionInfoboilerplateagentbase.StandardSessionInfoGetPromptDeliveryStrategy/GetConfigSpec/ tier-C no-op stubsagentbase.BaseembedfileExists×23hookutil.FileExistsCorrectness fix: the private
atomicWriteFilecopies in autohand/cline/goose/kiro omittedtmp.Sync(); routing them throughhookutil.AtomicWriteFilerestores the fsync before rename.Kept deliberately separate (per #2349)
Verification
go build ./...,go vet ./..., andgo test ./...all green frombackend/— 1,647 tests pass across 84 packages. Per-adapter public behavior (launch/restore argv, hook file contents, session info, binary resolution) is unchanged; test expectations were only migrated where they referenced now-shared internal types.🤖 Generated with Claude Code