refactor: extract shared detect+version-check pipeline#19
Merged
kevinelliott merged 1 commit intomainfrom Apr 21, 2026
Merged
Conversation
Consolidates the cache-aware detection and parallel latest-version fetch sequence into a new internal/orchestrator package. The four call sites that previously open-coded this flow (agent list, agent update, the TUI loader, and the systray refresh) now delegate to orchestrator.Pipeline.DetectAndCheckVersions, which removes the drift that previously required commit bf82ca8 to patch up agent update. Line-count changes across call sites: internal/cli/agent.go: +122 / -122 (list & update simplified) internal/tui/app.go: +18 / -78 loadDataWithRefresh internal/systray/systray.go: +15 / -68 refreshAgentsWithCache Net across the three edits: 57 insertions, 228 deletions (duplicated cache-logic collapsed into the new package). The new package itself is 255 lines of code plus 601 lines of tests (97.4% coverage). Behavior preserved: - agent list: drops version-check errors silently - agent update: aggregates errors and prints them (now passes ForceRefresh so the flow matches the original, which always ran a fresh detection before choosing updates) - TUI: surfaces fatal load errors via dataLoadedMsg{err}, swallows version-check errors - Systray: keeps the agentsMu write-lock swap and sets TolerateCatalogError=true to match the original tolerant startup Guardrails honored: - No changes to catalog.Manager, detector.Detector, installer.Manager, or storage.Store public types - internal/versionfetch is still the engine; Pipeline wraps it - pkg/api/rest and pkg/api/grpc keep their own cache helper (PR #16) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the repeated “detect installed agents + cache handling + latest-version checks” workflow into a shared internal/orchestrator pipeline and updates the CLI/TUI/systray call sites to delegate to it, aiming to keep behavior consistent across entry points while reducing duplication.
Changes:
- Added
internal/orchestratorwithPipeline.DetectAndCheckVersionsencapsulating cache-aware detection + parallel version checks (plus tests). - Refactored CLI (
agent list,agent update), TUI (loadDataWithRefresh), and systray (refreshAgentsWithCache) to use the pipeline. - Preserved the systray’s historical “tolerate catalog fetch failure” behavior via an option.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/orchestrator/pipeline.go | New shared pipeline implementing catalog load, detection-cache logic, version-check TTL, and best-effort persistence. |
| internal/orchestrator/pipeline_test.go | High-coverage tests validating cache hit/miss behavior, TTL handling, forced refresh, tolerated catalog errors, and error aggregation. |
| internal/cli/agent.go | Switches list/update commands to the pipeline; removes duplicated cache/version-check code paths. |
| internal/tui/app.go | Uses the pipeline for detection/version checks while still loading the full catalog for TUI rendering. |
| internal/systray/systray.go | Uses the pipeline in refresh flow while preserving “start even if catalog fetch fails”. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
138
to
140
| // Apply filters | ||
| var filtered []*agent.Installation | ||
| for _, inst := range installations { |
There was a problem hiding this comment.
In this agent list implementation, the showAll and checkUpdates flag values are never read (only bound via Flags().BoolVar...), so --all and --check-updates currently have no effect. Either implement their intended behavior (e.g., change filtering/output semantics and/or force a version check even when the update-check TTL is fresh) or remove the flags to avoid misleading CLI help output.
Comment on lines
+119
to
128
| pipelineRes, err := pipeline.DetectAndCheckVersions(ctx, orchestrator.Options{ | ||
| ForceRefresh: refresh, | ||
| }) | ||
| if err != nil { | ||
| spinner.Error("Agent detection failed") | ||
| return err | ||
| } | ||
| if pipelineRes.RanVersionCheck { | ||
| spinner.UpdateMessage("Checking for updates...") | ||
|
|
||
| // Fetch latest versions in parallel. Errors are intentionally | ||
| // dropped here to preserve the existing silent-failure semantics | ||
| // of `agent list`. | ||
| _ = versionfetch.CheckLatestVersions(ctx, instMgr, installations, agentDefMap, versionfetch.DefaultConcurrency) | ||
|
|
||
| // Save last update check time | ||
| _ = store.SetLastUpdateCheckTime(ctx, time.Now()) //nolint:errcheck // best-effort timestamp; non-critical if this fails | ||
|
|
||
| // Save to cache if enabled (with updated version info) | ||
| if cfg.Detection.CacheEnabled { | ||
| if err := store.SaveDetectionCache(ctx, installations); err != nil { | ||
| // Log but don't fail | ||
| _ = err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
pipeline.DetectAndCheckVersions(...) performs the version-check internally before returning, so updating the spinner to "Checking for updates..." only after the call completes is misleading (and typically flashes right before spinner.Stop()). Consider either using a single generic spinner message for the whole pipeline, or refactoring the pipeline API (e.g., split detect vs version-check, or add a progress hook) so the CLI can show the phase change while the work is actually happening.
| agentDefMap[def.ID] = def | ||
| } | ||
|
|
||
| spinner.UpdateMessage("Checking for updates...") |
There was a problem hiding this comment.
DetectAndCheckVersions already ran the version-check step before returning pipelineRes, so setting the spinner message to "Checking for updates..." at this point doesn’t reflect any ongoing work. Either remove this spinner phase, or adjust the orchestration so the spinner message changes before/while the version-check runs (e.g., call the pipeline with SkipVersionCheck and run versionfetch.CheckLatestVersions outside when you want a dedicated spinner phase).
Suggested change
| spinner.UpdateMessage("Checking for updates...") |
kevinelliott
added a commit
that referenced
this pull request
Apr 21, 2026
Promote the Unreleased section to 1.1.0 and capture everything landed since v1.0.24: the parallel version-check perf work (#16), the google.golang.org/grpc CVE-2026-33186 security bump (#17), the golangci-lint pinning (#18), and the shared detect+version-check pipeline refactor (#19). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
kevinelliott
added a commit
that referenced
this pull request
Apr 25, 2026
* fix: address Copilot review feedback — correctness fixes Seven real-bug fixes called out across review comments on merged PRs: - pkg/platform/platform.go: resetLookPathCache() no longer reassigns the package-level sync.Map (unsafe vs concurrent readers). Uses Range+Delete instead. - pkg/installer/providers/brew.go: GetLatestVersion no longer caches context-canceled / deadline-exceeded errors. A caller with a short ctx would have poisoned the 5m TTL for every later caller. - pkg/installer/providers/progress.go: the progress writer is now wrapped in a locking writer so concurrent Write calls from cmd.Stdout and cmd.Stderr (os/exec writes those from separate goroutines) don't interleave bytes mid-call. - pkg/catalog/embed.go: EmbeddedJSON() now returns a copy of the backing bytes. Previously callers could mutate the package-level []byte, corrupting the baseline for every future Get(). - pkg/catalog/manager.go: loadEmbedded now also probes /usr/share/agentmgr/catalog.json — where the goreleaser nfpm config installs the catalog for .deb/.rpm users. Previously that path was silently ignored at runtime even though goreleaser wrote the file there at install time. - internal/cli/doctor.go: Catalog Sources section updated with the matching /usr/share/agentmgr row so `doctor` output mirrors the loader's probe order. - internal/cli/agent.go: removed the --all (-a) and --check-updates flags from `agent list`. Both bound to local vars that were never read anywhere, so the flags did nothing; users setting them got a silent no-op instead of a useful behavior. Supersedes the corresponding Copilot annotations on PRs #19, #28, #29, #33, plus the dead-flag note on the agent list command. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: address remaining Copilot review nits (docs + tests) Batch of small review-comment fixes — documentation accuracy, test hardening, and gosec G104 on grpc teardown: - pkg/api/grpc/server.go: explicit \`_ = s.listener.Close()\` in Stop() and ForceStop() with a comment noting the close error is not actionable during teardown (silences gosec G104). - pkg/catalog/manager.go: Refresh doc now explicitly notes that singleflight does not merge contexts and the first caller's ctx governs the shared fetch — a caller with a short deadline can cancel the in-flight work for all shared callers. - pkg/installer/providers/progress.go: WithProgressWriter doc clarifies why passing nil is a no-op (asymmetry lets middleware attach a writer safely without downstream code accidentally clobbering it). - internal/systray/cli_uninstall_darwin.go: file is darwin-only via //go:build tag, so platform.ID() is always Darwin at runtime. Removed the dead Linux and Windows branches (the filepath/os/platform imports they needed are gone too). - CHANGELOG.md: "(Removed in [Unreleased].)" was a broken shortcut reference link — promoted to "(Removed in v1.2.0.)". - README.md: feature bullet uses the full "agentmgr agent list" command prefix, consistent with everywhere else in the doc. - internal/cli/output/spinner_test.go: non-TTY test replaced the 30ms sleep with a direct assertion on s.isTTY. Start() is a no-op when isTTY is false, so sleeping was measuring nothing. - internal/cli/output/table_test.go: ANSI-alignment loop now actually asserts inter-column padding instead of discarding each line. - internal/systray/handlers_test.go: mustParseVersion panics on parse error so test-setup bugs surface at the fixture, not downstream. TestDialogTracking uses bare *exec.Cmd values (trackDialog only checks pointer identity) to avoid the PATH lookup for \`true\` on Windows CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(cli): drop "all" flag assertion from list; assert removal The --all / -a and --check-updates flags were removed from \`agent list\` in the parent commit because both bound to unused local vars. Drop the existence assertion for "all" and add an anti-regression check that asserts both flags are ABSENT. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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
internal/orchestratorpackage exposingPipeline.DetectAndCheckVersions. The pipeline wrapsinternal/versionfetchand keeps every upstream concrete type (storage.Store,catalog.Manager,detector.Detector,installer.Manager) untouched.internal/cli/agent.go(agent list,agent update),internal/tui/app.go(Model.loadDataWithRefresh), andinternal/systray/systray.go(App.refreshAgentsWithCache) to delegate to the pipeline. Theagent refreshsubcommand and thepkg/api/rest/pkg/api/grpccache helpers are left alone per the guardrails.agent liststill drops version-check errors silently,agent updatestill aggregates and prints them, the TUI still swallows them intodataLoadedMsg{err}, and the systray still takes itsagentsMuwrite lock around the swap. Systray gets a newOptions.TolerateCatalogErrorknob so its historical behavior of starting up with an empty catalog when the network is down is preserved verbatim.Line-count impact per call site
+-internal/cli/agent.go(list+update)internal/tui/app.go(loadDataWithRefresh)internal/systray/systray.go(refreshAgentsWithCache)internal/orchestrator/pipeline.gointernal/orchestrator/pipeline_test.go(Numbers are approximate;
git diff --statcounts each line. The total shown by Git for the three modified files is+57 / -228, which matches the net shrink of the duplicated logic.)Test coverage
internal/orchestrator: 97.4% of statements, covering cold detect, cache hit (fresh & stale update-check TTL), force-refresh with cache-clear, skip-version-check, version-check error aggregation +NonNilErrorsfilter, fatal catalog + detector errors,TolerateCatalogErrorfallback, disabled cache, custom concurrency, and stale detection cache.go test ./... -race -short -count=1.golangci-lint run --timeout=5mat v1.64.8 is clean on all touched files. The only remaining finding is the pre-existinguninstallCLInolint:unuseddirective ininternal/systray/systray.go, which is untouched by this PR.Judgment calls (documented so reviewers don't need to re-derive them)
internal/vspkg/placement. The pipeline importsinternal/versionfetch, so it has to live underinternal/too. Promotingversionfetchwould have broadened the blast radius unnecessarily; leaving it in place keeps this refactor surgical.TolerateCatalogErroroption. The systray historically treated a failed catalog fetch as non-fatal (agentDefs = nilthen proceed). The other call sites treated it as fatal. Rather than silently change either behavior, I added a per-call option and flipped it on only in the systray.agent updateand the detection cache. The pre-refactoragent updatealways ran a freshDetectAll, but did not write back to the detection cache. The pipeline does write back after a cold detect (systray reference behavior). This is a strict improvement — subsequentagent listcalls see the fresh versions — and is explicitly called out in the commit message.agent refreshkept out of scope. The task brief listed onlyagent list,agent update, TUI, and systray.newAgentRefreshCommandhas its own slightly different contract (always clears the cache, always detects), so I left it alone to honor the "surgical" guardrail.Test plan
go build ./...go vet ./...go test ./... -race -short -count=1/tmp/gcil/golangci-lint run --timeout=5m(v1.64.8, matches CI)go test -cover ./internal/orchestrator/...— 97.4%agentmgr agent list,agentmgr agent list -r,agentmgr agent update --all --dry-run, and the TUI (agentmgr tui) to confirm no regressions in spinner messages or update detectionTolerateCatalogErrorCo-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com