Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
221 changes: 221 additions & 0 deletions cmd/ci/agent_versions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,221 @@
package ci

import (
"context"
"errors"
"fmt"
"log/slog"
"net/http"
"os"
"path"
"strings"
"time"

"github.com/codefly-dev/cli/pkg/cli"
"github.com/codefly-dev/core/agents/manager"
"github.com/codefly-dev/core/resources"
)

// Seams for testing the artifact probe without reaching real registries.
var (
resolveAgentLatest = manager.ResolveLatest
agentAlreadyLocal = manager.Downloaded
githubAssetURL = manager.DownloadURL
agentProbeClient = &http.Client{Timeout: 20 * time.Second}
)

// agentSourceProbe is the outcome of resolving one agent against one source.
type agentSourceProbe struct {
label string
downloadable bool
detail string
}

// agentArtifactStatus aggregates every source tried for a single agent pin.
type agentArtifactStatus struct {
agent *resources.Agent
sources []agentSourceProbe
}

func (s agentArtifactStatus) downloadable() bool {
for _, source := range s.sources {
if source.downloadable {
return true
}
}
return false
}

// validateAgentVersions resolves every affected service's pinned agent against
// the configured artifact sources before any CI phase spawns it. A pin that is
// tagged but has no downloadable artifact would otherwise surface as an opaque
// 404 deep inside a phase; here it fails fast with a single, legible report.
//
// Skipped entirely under CODEFLY_AGENT_SOURCE=local, where agents are resolved
// from local builds and no artifact is ever downloaded.
func validateAgentVersions(ctx context.Context, workspace *resources.Workspace, plan *Plan) error {
if manager.AgentSourceLocal() {
return nil
}
if plan == nil || len(plan.Services) == 0 {
return nil
}
cli.Header(2, "Pre-flight: validating agent versions")

agents, err := collectPlanAgents(ctx, workspace, plan)
if err != nil {
return err
}

var unpublished []agentArtifactStatus
for _, agent := range agents {
if _, err := resolveAgentLatest(ctx, agent); err != nil {
unpublished = append(unpublished, agentArtifactStatus{
agent: agent,
sources: []agentSourceProbe{{label: "version resolution", detail: err.Error()}},
})
continue
Comment on lines +72 to +77

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Any version-resolution failure is currently reported as “not downloadable / no published artifact,” but resolution errors can also be transient/environmental (network/auth/config) and are not equivalent to an unpublished pin. Return a distinct error path for resolution failures so the report does not misclassify the root cause. [logic error]

Severity Level: Major ⚠️
- ⚠️ Network or auth issues misreported as unpublished agent pins.
- ⚠️ CI users may chase incorrect “publish artifact” fixes.
- ⚠️ Harder to distinguish real missing releases from infra issues.
Steps of Reproduction ✅
1. Run `codefly ci run` so `RunCmd.RunE` in `cmd/ci/run.go:22-86` executes and constructs
a non-empty `plan` with services that have agents (`Plan.BuildPlan` and
`collectPlanAgents` in `cmd/ci/agent_versions.go:94-123`).

2. Within the `runWithCIReport` callback in `cmd/ci/run.go:54-85`,
`validateAgentVersions(ctx, workspace, plan)` is called before any phase runs, entering
the loop over agents at `cmd/ci/agent_versions.go:70-86`.

3. For an agent whose version cannot be resolved (i.e., `resolveAgentLatest(ctx, agent)`
returns a non-nil error for any reason such as network, auth, or configuration), the code
at `cmd/ci/agent_versions.go:72-77` appends an `agentArtifactStatus` whose only source is
`{label: "version resolution", detail: err.Error()}` and classifies this under the
`unpublished` slice, then continues without distinguishing transient failures from truly
unpublished pins.

4. After the loop, `formatUnpublishedReport(unpublished)` is called
(`cmd/ci/agent_versions.go:88-92`), producing a report whose header and per-agent lines
state that the agent pins are “not downloadable in CI (no published artifact)” and “is not
published (no CI-downloadable artifact)” (`cmd/ci/agent_versions.go:212-215`, asserted in
`cmd/ci/agent_versions_test.go:114-131`), so any resolution error—regardless of cause—is
surfaced to users as a “no published artifact” problem rather than as a distinct
resolution or environment failure.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** cmd/ci/agent_versions.go
**Line:** 72:77
**Comment:**
	*Logic Error: Any version-resolution failure is currently reported as “not downloadable / no published artifact,” but resolution errors can also be transient/environmental (network/auth/config) and are not equivalent to an unpublished pin. Return a distinct error path for resolution failures so the report does not misclassify the root cause.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

}
if local, err := agentAlreadyLocal(ctx, agent); err == nil && local {
continue
}
Comment on lines +79 to +81

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The local-agent presence check suppresses all errors and treats them as “not local,” which can mask filesystem/configuration failures and produce misleading unpublished-artifact reports. Propagate or explicitly handle the agentAlreadyLocal error instead of silently ignoring it. [incorrect condition logic]

Severity Level: Major ⚠️
- ⚠️ Local cache errors hidden behind remote probe behavior.
- ⚠️ Unpublished-agent report omits underlying local failure context.
- ⚠️ Debugging CI agent issues harder for affected services.
Steps of Reproduction ✅
1. Invoke the `codefly ci run` CLI entrypoint implemented in `cmd/ci/run.go:22-86` (e.g.,
`codefly ci run`) so that `RunCmd.RunE` executes.

2. Inside `RunCmd.RunE`, after building the plan and phases (`cmd/ci/run.go:45-52`),
`runWithCIReport` is called with a callback that unconditionally calls
`validateAgentVersions(ctx, workspace, plan)` (`cmd/ci/run.go:54-57`).

3. In `validateAgentVersions` (`cmd/ci/agent_versions.go:56-92`), each collected agent is
processed; after successful `resolveAgentLatest`, the code checks local presence via
`agentAlreadyLocal(ctx, agent)` aliased to `manager.Downloaded`
(`cmd/ci/agent_versions.go:21-23, 79-81`).

4. When `manager.Downloaded` returns a non-nil error for a given agent (for example due to
a filesystem or local cache/configuration problem), the condition `err == nil && local` on
line 79 is false, the error is silently discarded (no logging or wrapping), and the loop
continues to remote probing as if the agent were simply “not local”, masking the original
local failure and leading any later report (from `formatUnpublishedReport` at
`cmd/ci/agent_versions.go:206-221`) to omit the real local error cause.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** cmd/ci/agent_versions.go
**Line:** 79:81
**Comment:**
	*Incorrect Condition Logic: The local-agent presence check suppresses all errors and treats them as “not local,” which can mask filesystem/configuration failures and produce misleading unpublished-artifact reports. Propagate or explicitly handle the `agentAlreadyLocal` error instead of silently ignoring it.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

status := probeAgentArtifact(ctx, agent)
if !status.downloadable() {
unpublished = append(unpublished, status)
}
}

if len(unpublished) == 0 {
return nil
}
return errors.New(formatUnpublishedReport(unpublished))
}

// collectPlanAgents returns the distinct agents backing the affected services,
// keyed by kind+identifier so a shared pin is validated once.
func collectPlanAgents(ctx context.Context, workspace *resources.Workspace, plan *Plan) ([]*resources.Agent, error) {
seen := map[string]bool{}
var agents []*resources.Agent
for _, planned := range plan.Services {
ref, err := resources.ParseServiceWithOptionalModule(planned.Service)
if err != nil {
return nil, fmt.Errorf("parse affected service %q: %w", planned.Service, err)
}
module, err := workspace.LoadModuleFromName(ctx, ref.Module)
if err != nil {
return nil, fmt.Errorf("load module %q: %w", ref.Module, err)
}
service, err := module.LoadServiceFromName(ctx, ref.Name)
if err != nil {
return nil, fmt.Errorf("load service %q: %w", planned.Service, err)
}
if service.Agent == nil {
continue
}
agent := *service.Agent
if seen[agent.Unique()] {
continue
}
seen[agent.Unique()] = true
agents = append(agents, &agent)
}
return agents, nil
}

// probeAgentArtifact HEADs the GitHub release asset and, when configured, the
// OCI manifest and Nix flake output for the agent.
func probeAgentArtifact(ctx context.Context, agent *resources.Agent) agentArtifactStatus {
status := agentArtifactStatus{agent: agent}
status.sources = append(status.sources, probeGitHubAsset(ctx, agent))
status.sources = append(status.sources, probeOCIManifest(ctx, agent))
if source, ok := probeNixFlake(ctx, agent); ok {
status.sources = append(status.sources, source)
}
return status
}

func probeGitHubAsset(ctx context.Context, agent *resources.Agent) agentSourceProbe {
url := githubAssetURL(agent)
probe := agentSourceProbe{label: "GitHub release asset " + path.Base(url)}
req, err := http.NewRequestWithContext(ctx, http.MethodHead, url, nil)
if err != nil {
probe.detail = err.Error()
return probe
}
resp, err := agentProbeClient.Do(req)
if err != nil {
probe.detail = err.Error()
return probe
}
defer resp.Body.Close()
if resp.StatusCode == http.StatusOK {
probe.downloadable = true
probe.detail = "ok"
return probe
}
probe.detail = fmt.Sprintf("%d", resp.StatusCode)
return probe
}

func probeOCIManifest(ctx context.Context, agent *resources.Agent) agentSourceProbe {
registry := strings.TrimSpace(os.Getenv("AGENT_REGISTRY"))
if registry == "" {
return agentSourceProbe{label: "OCI registry", detail: "not configured (set AGENT_REGISTRY)"}
}
reference := fmt.Sprintf("%s/agents/%s/%s:%s", registry, agent.Publisher, agent.Name, agent.Version)
probe := agentSourceProbe{label: "OCI " + reference}
store := manager.NewOCIStoreFromEnv(slog.Default())
if store == nil {
probe.detail = "not configured (set AGENT_REGISTRY)"
return probe
}
available, err := store.Available(ctx, agent)
if err != nil {
probe.detail = err.Error()
return probe
}
if available {
probe.downloadable = true
probe.detail = "ok"
return probe
}
probe.detail = "manifest not found"
return probe
}

func probeNixFlake(ctx context.Context, agent *resources.Agent) (agentSourceProbe, bool) {
store := manager.NewNixStoreFromEnv(slog.Default())
if store == nil {
return agentSourceProbe{}, false
}
probe := agentSourceProbe{label: "Nix flake " + os.Getenv("AGENT_NIX_FLAKE")}
available, err := store.Available(ctx, agent)
if err != nil {
probe.detail = err.Error()
return probe, true
}
if available {
probe.downloadable = true
probe.detail = "ok"
return probe, true
}
probe.detail = "flake output not found"
return probe, true
}

func formatUnpublishedReport(statuses []agentArtifactStatus) string {
var b strings.Builder
plural := "pin"
if len(statuses) > 1 {
plural = "pins"
}
fmt.Fprintf(&b, "%d agent %s not downloadable in CI (no published artifact):\n", len(statuses), plural)
for _, status := range statuses {
fmt.Fprintf(&b, "\nagent %s is not published (no CI-downloadable artifact)\n", status.agent.Identifier())
for _, source := range status.sources {
fmt.Fprintf(&b, " - %s: %s\n", source.label, source.detail)
}
b.WriteString(" -> tag + release the agent, or set AGENT_REGISTRY\n")
}
return strings.TrimRight(b.String(), "\n")
}
147 changes: 147 additions & 0 deletions cmd/ci/agent_versions_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
package ci

import (
"context"
"net/http"
"net/http/httptest"
"strings"
"testing"

"github.com/codefly-dev/core/resources"
)

func testAgent() *resources.Agent {
return &resources.Agent{
Kind: resources.ServiceAgent,
Publisher: "codefly.dev",
Name: "redis",
Version: "0.0.74",
}
}

func TestProbeGitHubAssetReportsMissingArtifact(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusNotFound)
}))
defer server.Close()

restore := githubAssetURL
githubAssetURL = func(*resources.Agent) string { return server.URL + "/service-redis_0.0.74_linux_amd64.tar.gz" }
defer func() { githubAssetURL = restore }()

probe := probeGitHubAsset(context.Background(), testAgent())
if probe.downloadable {
t.Fatal("404 asset reported as downloadable")
}
if probe.detail != "404" {
t.Fatalf("detail = %q, want 404", probe.detail)
}
if !strings.Contains(probe.label, "service-redis_0.0.74_linux_amd64.tar.gz") {
t.Fatalf("label = %q, want the asset filename", probe.label)
}
}

func TestProbeGitHubAssetReportsPublishedArtifact(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusOK)
}))
defer server.Close()

restore := githubAssetURL
githubAssetURL = func(*resources.Agent) string { return server.URL + "/asset.tar.gz" }
defer func() { githubAssetURL = restore }()

probe := probeGitHubAsset(context.Background(), testAgent())
if !probe.downloadable {
t.Fatalf("200 asset reported as not downloadable: %q", probe.detail)
}
}

func TestProbeOCIManifestNotConfigured(t *testing.T) {
t.Setenv("AGENT_REGISTRY", "")
probe := probeOCIManifest(context.Background(), testAgent())
if probe.downloadable {
t.Fatal("unconfigured OCI reported as downloadable")
}
if !strings.Contains(probe.detail, "not configured") {
t.Fatalf("detail = %q, want 'not configured'", probe.detail)
}
}

func TestProbeOCIManifestAvailable(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.Method == http.MethodHead && strings.Contains(r.URL.Path, "/manifests/0.0.74") {
w.WriteHeader(http.StatusOK)
return
}
w.WriteHeader(http.StatusNotFound)
}))
defer server.Close()

registry := strings.TrimPrefix(server.URL, "http://")
t.Setenv("AGENT_REGISTRY", registry)

probe := probeOCIManifest(context.Background(), testAgent())
if !probe.downloadable {
t.Fatalf("available OCI manifest reported as not downloadable: %q", probe.detail)
}
want := registry + "/agents/codefly.dev/redis:0.0.74"
if !strings.Contains(probe.label, want) {
t.Fatalf("label = %q, want reference %q", probe.label, want)
}
}

func TestProbeAgentArtifactCombinesSources(t *testing.T) {
github := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusNotFound)
}))
defer github.Close()

restore := githubAssetURL
githubAssetURL = func(*resources.Agent) string { return github.URL + "/asset.tar.gz" }
defer func() { githubAssetURL = restore }()
t.Setenv("AGENT_REGISTRY", "")

status := probeAgentArtifact(context.Background(), testAgent())
if status.downloadable() {
t.Fatal("agent with no reachable source reported as downloadable")
}
if len(status.sources) != 2 {
t.Fatalf("sources = %d, want GitHub + OCI when Nix is unconfigured", len(status.sources))
}
}

func TestFormatUnpublishedReportListsEverySource(t *testing.T) {
statuses := []agentArtifactStatus{
{
agent: testAgent(),
sources: []agentSourceProbe{
{label: "GitHub release asset service-redis_0.0.74_linux_amd64.tar.gz", detail: "404"},
{label: "OCI registry", detail: "not configured (set AGENT_REGISTRY)"},
},
},
}
report := formatUnpublishedReport(statuses)
for _, want := range []string{
"1 agent pin not downloadable in CI",
"agent codefly.dev/redis:0.0.74 is not published",
"GitHub release asset service-redis_0.0.74_linux_amd64.tar.gz: 404",
"OCI registry: not configured (set AGENT_REGISTRY)",
"-> tag + release the agent, or set AGENT_REGISTRY",
} {
if !strings.Contains(report, want) {
t.Fatalf("report missing %q\n%s", want, report)
}
}
}

func TestFormatUnpublishedReportPluralizes(t *testing.T) {
statuses := []agentArtifactStatus{
{agent: testAgent(), sources: []agentSourceProbe{{label: "GitHub", detail: "404"}}},
{agent: &resources.Agent{Kind: resources.ServiceAgent, Publisher: "codefly.dev", Name: "vault", Version: "0.0.15"}, sources: []agentSourceProbe{{label: "GitHub", detail: "404"}}},
}
report := formatUnpublishedReport(statuses)
if !strings.HasPrefix(report, "2 agent pins not downloadable in CI") {
t.Fatalf("report = %q, want plural header", report)
}
}
3 changes: 3 additions & 0 deletions cmd/ci/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ var RunCmd = &cobra.Command{
return err
}
return runWithCIReport(ctx, workspace, plan, "codefly ci run", func(reporter *CIReporter) error {
if err := validateAgentVersions(ctx, workspace, plan); err != nil {
return err
}
Comment on lines +55 to +57

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The pre-flight validation is now unconditional for every ci run invocation, which will fail runs that only execute non-agent phases (for example verify) due to unrelated artifact/network checks. Gate this validation so it only runs when at least one selected phase actually needs service agents. [incomplete implementation]

Severity Level: Major ⚠️
- ⚠️ `codefly ci run --phase verify` can fail on agents.
- ⚠️ Workspace integrity checks blocked by unrelated agent publishing.
- ⚠️ CI pipeline for verify-only gates becomes unnecessarily fragile.
Steps of Reproduction ✅
1. Execute `codefly ci run --phase verify` (or an equivalent Cobra invocation) so that
`RunCmd.RunE` in `cmd/ci/run.go:22-86` is entered, `runPhases` contains only `"verify"`,
and `plan` is built for the workspace services (`cmd/ci/run.go:45-48`).

2. `normalizeRunPhases` at `cmd/ci/run.go:171-192` returns a `phases` slice containing
only `"verify"`, which is then passed into the `runWithCIReport` callback at
`cmd/ci/run.go:54-85`.

3. Before any phase-specific logic runs, the callback calls `validateAgentVersions(ctx,
workspace, plan)` unconditionally at `cmd/ci/run.go:55-57`; inside `validateAgentVersions`
(`cmd/ci/agent_versions.go:56-92`), every agent pin is resolved and probed against
GitHub/OCI/Nix, and if any pin lacks a downloadable artifact or hits a transient error, an
error report is returned immediately.

4. Because `runCIPhases` is only invoked after this callback completes successfully
(`cmd/ci/run.go:81-84`), a `--phase verify` run will fail early on agent artifact/network
issues even though the only selected phase, `verify`, is implemented by
`runVerifyWorkspace` (`cmd/ci/run.go:120-123`) which in turn calls `integrity.VerifyBase`
over module manifests (`pkg/integrity/base.go:53-132`) and does not depend on service
agents, meaning verify-only runs are incorrectly gated on agent artifact availability.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** cmd/ci/run.go
**Line:** 55:57
**Comment:**
	*Incomplete Implementation: The pre-flight validation is now unconditional for every `ci run` invocation, which will fail runs that only execute non-agent phases (for example `verify`) due to unrelated artifact/network checks. Gate this validation so it only runs when at least one selected phase actually needs service agents.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

suites := normalizeTestSuites(testSuites)
for _, phase := range phases {
if phase == "verify" {
Expand Down
Loading