RTECO-1411 - Implement integration tests for Agent Plugins

[udaykb2]

• All tests have passed. If this feature is not already covered by the tests, new tests have been added.
• The pull request is targeting the master branch.
• The code has been validated to compile successfully by running go vet ./....
• The code has been formatted properly using go fmt ./....

---

[Udaysonu]

Code Review: RTECO-1411 — Agent Plugins Integration Tests

Files reviewed: 8 files (3 production-support, 5 test/fixture)

Executive Summary

This PR adds a solid and well-structured integration test suite for Agent Plugins. The test coverage breadth is impressive, and the helper utilities (createTestPlugin, assertPluginExists/Absent) follow established patterns well. However there are several issues that need attention before merge: a potential CI secret-exfiltration vector via pull_request_target, an infinite CI hang risk from --timeout 0, a test that effectively asserts nothing (TestAgentPluginsInstallEvidenceGateCI), a test that misrepresents what it covers (TestAgentPluginsArtifactoryUnreachable), and a setup helper that silently swallows copy errors leading to confusing downstream failures.

---

CRITICAL

[CRITICAL] .github/workflows/agentPluginsTests.yml:11,38 — pull_request_target + forked HEAD checkout + secrets = secret exfiltration risk

The workflow uses pull_request_target (which runs with write access and exposes repository secrets including RTLIC) and simultaneously checks out the forked PR's HEAD SHA (github.event.pull_request.head.sha). This is the classic "pwn-request" pattern: a malicious PR can modify workflow scripts or test code that then executes with access to your RTLIC secret. The safe to test label guard reduces (but does not eliminate) the risk, because a maintainer who applies the label without carefully reviewing all changed files still triggers secret exposure. Either use a repository-dispatch approach (safe pattern used by many OSS projects), or restrict the checkout to github.sha (the base branch) for pull_request_target events, and run tests in a separate non-secret job.

[CRITICAL] .github/workflows/agentPluginsTests.yml:56 — --timeout 0 disables Go test timeout entirely

If any test hangs (network stall, deadlock, missing fixture), the CI job runs indefinitely and blocks the runner queue. Other PRs in the repo share those runners. Use a bounded timeout such as --timeout 30m or --timeout 45m that gives tests generous time without creating an infinite blocking scenario.

---

MAJOR

[MAJOR] agent_plugins_test.go:142 — assert.NoError in setup helper swallows copy failure

createTestPlugin uses assert.NoError(t, biutils.CopyDir(...)) (non-fatal) for the directory copy but then require.NoError for subsequent writes. If CopyDir fails, the function returns a path to an empty directory; every downstream test using the helper will fail with a confusing "plugin.json not found" or upload error rather than the real root cause. Change to require.NoError to fail fast.

[MAJOR] agent_plugins_test.go:1074 — TestAgentPluginsInstallEvidenceGateCI is a no-op assertion in standard setups

The test body explicitly states "The command may succeed or fail depending on whether evidence enforcement is active (Enterprise+). Either outcome is valid." An assertion that accepts both pass and fail is not testing anything. On a standard Artifactory instance this test always passes regardless of whether the evidence gate logic is implemented correctly. Either skip the test entirely when the evidence service is not configured, or restructure it into two separate tests with definite pass/fail expectations conditioned on service availability detection.

[MAJOR] agent_plugins_test.go:1707 — TestAgentPluginsArtifactoryUnreachable tests the wrong failure path

The test declares bogusServerURL and immediately discards it (_ = bogusServerURL // used conceptually), then uses --server-id=nonexistent-server-id-xyz to trigger failure. This fails because the server ID is unknown to the local config — identical to TestAgentPluginsServerIDUnknown — NOT because Artifactory is network-unreachable. Scenario #79 (unreachable host) is never exercised. To actually test unreachability, configure a server ID pointing to bogusServerURL before running the command.

[MAJOR] agent_plugins_test.go:906 — TestAgentPluginsInstallGlobal does not test --global — it tests --path

The comment says it covers scenario #26 (--global install) but the command uses --path=globalBase rather than --global. The --path flag is already exercised by TestAgentPluginsInstallLatest and TestAgentPluginsInstallWithPath. To actually test --global, override HOME with t.Setenv("HOME", t.TempDir()) and use the --global flag.

[MAJOR] agent_plugins_test.go:1601 — TestAgentPluginsChecksumRoundTrip does not verify a round-trip

The test name and docstring promise "the local SHA256 of the installed zip matches what Artifactory stored" but the implementation: (1) never installs the plugin locally, (2) never computes a local hash, (3) only checks item.Sha256 != "". This is identical to what TestAgentPluginsChecksumIntegrity already verifies. Either rename the test to TestAgentPluginsChecksumStoredByArtifactory, or complete the round-trip by installing the plugin, computing sha256.Sum256 on the downloaded zip, and comparing against item.Sha256.

---

MINOR

[MINOR] .github/workflows/agentPluginsTests.yml — macOS missing from runner matrix

The matrix covers only Ubuntu and Windows. Plugin path construction, zip handling, and home-directory resolution can behave differently on macOS. Existing CLI test suites include macOS. Consider adding macos-14 or macos-15, or document why it is intentionally excluded.

[MINOR] agent_plugins_test.go:176 — assertPluginAbsent iterates the full result set after found = true

Once found = true the loop continues draining the reader unnecessarily. Add break after found = true to release the reader connection sooner.

[MINOR] agent_plugins_test.go:249 — assert.NoError for publish in TestAgentPluginsPublishWithVersion should be require.NoError

All other publish steps in the P0 test suite use require.NoError. If publish fails, assertPluginExists is still called and produces a secondary confusing error about a missing artifact rather than the real publish failure.

[MINOR] agent_plugins_test.go:1761 — double cleanup via both callback and defer in TestAgentPluginsNoProxy

clientTestUtils.SetEnvWithCallbackAndAssert registers the restore function via t.Cleanup internally. The explicit defer restoreNoProxy() runs the same restore function a second time. Remove the defer line.

[MINOR] agent_plugins_test.go:839 — prebuilt zip test uses a fake/invalid zip

[]byte("PK\x03\x04fake-zip-content") starts with a valid ZIP magic but has no central directory. If Artifactory or the upload layer validates zip structure, the test will fail with an opaque server error rather than a clean assertion failure. Use archive/zip to create a minimal valid zip in-memory.

---

NIT

[Nit] agent_plugins_test.go:1708 — unused bogusServerURL variable should be removed — see MAJOR finding above.

[Nit] Output-format tests only assert no-error, not output correctness — TestAgentPluginsInstallFormatJSON, TestAgentPluginsSearchFormatJSON, and the format-json case in TestAgentPluginsListFlags check that the command exits cleanly but do not verify that the output is actually valid JSON. These tests cannot detect a regression where --format json is silently ignored.

---

Positive Highlights

• Excellent use of t.Cleanup and t.TempDir() throughout — no manual teardown of temp files needed.
• createTestPlugin / createTestPluginWithVersion / createTestPluginWithSlug helper triad is clean and reusable, avoiding fixture duplication.
• The scenario coverage mapping ("Covers scenario #N") in every test docstring is excellent practice for traceability.
• assertPluginAbsent correctly uses a search-based approach rather than relying on GetItemProps returning nil on 404 — well-commented.
• TestAgentPluginsUpdateFlags table-driven approach for mutually exclusive flag validation is idiomatic and easy to extend.
• Correct use of t.Setenv (rather than os.Setenv) ensures env vars are restored between tests.

Verdict

REQUEST CHANGES

The pull_request_target + forked-HEAD checkout pattern is a CI security issue that must be resolved. The --timeout 0 hang risk and the three test-correctness issues (no-op evidence gate, wrong failure path in unreachable test, missing round-trip in checksum test) should also be addressed before merge.

[Udaysonu]

[CRITICAL] Security: pull_request_target + forked HEAD checkout + secrets

This is the classic "pwn-request" attack vector. pull_request_target runs with write permissions and exposes RTLIC and FASTCI_TOKEN to the workflow. Checking out the forked PR's HEAD SHA (line 38) means a malicious PR author can execute arbitrary code with access to those secrets.

The safe to test label guard reduces but does not eliminate the risk — a maintainer applying the label without scrutinising every changed file still grants secret access.

Safe pattern: for pull_request_target events, always check out github.sha (the base branch), never the fork head SHA. Or use a repository-dispatch approach where a separate, secret-free job validates the label then triggers a dispatch event.

[Udaysonu]

[CRITICAL] Checking out forked PR HEAD under pull_request_target

When triggered by pull_request_target with the labeled event type, github.event.pull_request.head.sha resolves to the fork's commit. Combined with access to repository secrets (RTLIC), this is a confirmed secret-exfiltration vector (see GitHub Security Advisory GHSA-p2g7-xwvr-rrw3 and related). See also the comment on line 11.

[Udaysonu]

[CRITICAL] --timeout 0 disables Go test timeout entirely

If any test hangs (network stall, deadlock, missing prerequisite), the runner job blocks indefinitely and starves other PRs waiting for a runner slot.

Suggested: Replace --timeout 0 with a bounded value, e.g. --timeout 45m. Other CLI integration test jobs in this repo can be used as a reference for appropriate values.

[Udaysonu]

[MAJOR] assert.NoError in setup helper should be require.NoError

If biutils.CopyDir fails, the function returns a path to an incomplete (or empty) temp directory. Every test that calls createTestPlugin will then fail later with a confusing "plugin.json missing" or upload error, hiding the real root cause.

// Before
assert.NoError(t, biutils.CopyDir(pluginSrc, pluginPath, true, nil))
// After
require.NoError(t, biutils.CopyDir(pluginSrc, pluginPath, true, nil))

[Udaysonu]

[Minor] Break after found = true to avoid draining the full result set

Once found is set the loop continues calling NextRecord unnecessarily, holding the reader connection open longer than needed.

for item := new(artUtils.SearchResult); reader.NextRecord(item) == nil; item = new(artUtils.SearchResult) {
    found = true
    break
}

[Udaysonu]

[MAJOR] TestAgentPluginsInstallGlobal does not test --global — it tests --path

The test comment claims coverage of scenario #26 (--global install), but the command passes --path=globalBase. The --path flag is already exercised by TestAgentPluginsInstallLatest and TestAgentPluginsInstallWithPath.

To actually test --global, override the home directory so CI runners are not polluted:

homeDir := t.TempDir()
t.Setenv("HOME", homeDir)       // Linux/macOS
t.Setenv("USERPROFILE", homeDir) // Windows
assert.NoError(t, runAgentPluginsCmd(t,
    "install", slug,
    "--repo="+tests.AgentPluginsLocalRepo,
    "--global",
))
// verify under homeDir according to the harness spec

[Udaysonu]

[MAJOR] Test accepts both pass and fail — it asserts nothing in standard setups

// The command may succeed or fail depending on whether evidence enforcement
// is active (Enterprise+). Either outcome is valid; what matters is no panic.

A test that declares both outcomes valid provides zero regression protection. On a standard Artifactory instance this test always passes regardless of whether the evidence gate is implemented correctly.

Options:

1. Skip explicitly when evidence service is not configured (via a capability-check helper or a dedicated skip env var).
2. Split into two tests: one for Enterprise+ (asserts failure + specific message), one for standard (asserts success).
3. At minimum, assert require.NoError so the test has a definitive expectation on standard test infrastructure.

[Udaysonu]

[MAJOR] TestAgentPluginsChecksumRoundTrip does not perform a round-trip

The test name and docstring promise "the local SHA256 of the installed zip matches what Artifactory stored", but the implementation:

1. Never installs the plugin locally
2. Never computes a local hash
3. Only asserts item.Sha256 != "" — identical to TestAgentPluginsChecksumIntegrity

Either:

• Rename to TestAgentPluginsChecksumStoredByArtifactory and update the docstring to match what the test actually checks, or
• Complete the round-trip: install the plugin, locate the downloaded zip, compute sha256.Sum256, and compare to item.Sha256.

[Udaysonu]

[MAJOR] bogusServerURL is declared and immediately discarded — and tests the wrong failure path

bogusServerURL := "https://nonexistent-artifactory-host-xyzzy.example.com/artifactory/"
_ = bogusServerURL // used conceptually — actual routing depends on server config

This test fails because nonexistent-server-id-xyz is not in the local JFrog config — the exact same failure path as TestAgentPluginsServerIDUnknown. Scenario #79 (network-unreachable Artifactory) is never exercised.

To actually test unreachability, first configure a server pointing to bogusServerURL, then run the command with that server ID. Remove the unused bogusServerURL variable regardless of whether the broader fix is made.

[Udaysonu]

[Minor] Double cleanup — defer restoreNoProxy() is redundant

clientTestUtils.SetEnvWithCallbackAndAssert already registers the restore via t.Cleanup. The explicit defer restoreNoProxy() causes the same function to run twice (defer executes before t.Cleanup). The second invocation may attempt to restore an already-restored env var.

Remove the defer restoreNoProxy() line and rely solely on the t.Cleanup-registered callback.

[Udaysonu]

Code Review: RTECO-1411 — Integration Tests for Agent Plugins

Files reviewed: 8 files (1 workflow, 1 main test file, 3 test fixtures, 2 utils files, 1 test data config)

Executive Summary

This PR adds a well-structured, comprehensive integration test suite for the Agent Plugins feature. Coverage is broad (publish, install, update, delete, search, list, build info, TLS, proxy, round-trip) and the test helpers are clean and reusable. However, there is one Critical security issue in the CI workflow, two Major correctness problems, and several Minor/Nit issues that should be addressed before merge.

---

Findings

Critical

Security — pull_request_target + PR head checkout (agentPluginsTests.yml:38)
pull_request_target runs with repository write permissions and has access to secrets (including RTLIC). Checking out github.event.pull_request.head.sha means a malicious PR author can submit a modified test that exfiltrates secrets. The safe to test label guard prevents automatic triggering but does not prevent the attack once a maintainer applies the label — and a compromised maintainer account bypasses it entirely. Fix: Either (a) only checkout github.ref (base branch) under pull_request_target, or (b) switch to the pull_request event which has no write permissions.

Major

Testing — assert.NoError for biutils.CopyDir should be require.NoError (agent_plugins_test.go:74)
If CopyDir fails (missing fixture, permissions), execution continues with a partially-written pluginPath. All downstream assertions then fail at unrelated points with confusing messages. require.NoError stops the test immediately at the failure site.

Testing — TestAgentPluginsArtifactoryUnreachable tests the wrong scenario (agent_plugins_test.go:1639)
bogusServerURL is assigned and immediately discarded (_ = bogusServerURL). The test only exercises an unknown --server-id flag, which is identical to TestAgentPluginsServerIDUnknown. Scenario #79 (actual network unreachability — DNS failure, TCP timeout) is not exercised at all. Either remove this test (it is a duplicate) or implement it correctly by registering a JFrog CLI server profile pointing to an unreachable URL.

Minor

Testing — Misleading comment in assertPluginAbsent (agent_plugins_test.go:97)
The comment states GetItemProps returns nil on 404 but GetItemProps returns an error on 404, not nil. The actual justification for the search-based approach should be stated accurately.

Testing — TestAgentPluginsInstallEvidenceGateCI is non-assertive on success (agent_plugins_test.go:1008)
The if err != nil { ... } pattern means: if the evidence gate is not enforced on the test Artifactory instance, the test passes trivially and validates nothing. Consider adding a t.Log or t.Skip to make the bypass explicit so it is clear when the path was not exercised.

Testing — Fake zip in TestAgentPluginsPublishPrebuiltZip may cause unexpected failure (agent_plugins_test.go:771)
[]byte("PK\x03\x04fake-zip-content") is not a valid zip file (only the 4-byte magic is present). If the publish command validates zip integrity before upload, this test will fail with a confusing zip-format error rather than exercising the prebuilt path. Use archive/zip to generate a minimal valid (but empty) zip instead.

CI — Unpinned @main action references (agentPluginsTests.yml:47)
jfrog/.github/actions/install-go-with-cache@main and jfrog/.github/actions/install-local-artifactory@main use mutable refs. A breaking change to those actions silently breaks this workflow. Pin to a versioned tag or commit SHA.

Nit

Multiple build-info tests hardcode buildNumber = "1"
Tests TestAgentPluginsChecksumIntegrity, TestAgentPluginsPublishWithBuildInfo, TestAgentPluginsModuleOverride, TestAgentPluginsBuildPropertiesOnArtifact, TestAgentPluginsBuildPublishRetrievable, and TestAgentPluginsCIPipeline all share buildNumber = "1" with the same build name. Sequential execution with cleanup makes this safe today, but using t.Name() or a derived counter would make tests fully independent of execution order.

--timeout 0 disables test timeout entirely (agentPluginsTests.yml:56)
A deadlocked test will hang the CI runner indefinitely and require manual cancellation. A generous but finite value like --timeout 2h is safer.

---

Positive Highlights

• Excellent section dividers with scenario number references — the file is easy to navigate and extend.
• initAgentPluginsTest proactively disabling the quiet-failure evidence gate via t.Setenv is the right pattern for a test Artifactory without the evidence service.
• assertPluginAbsent using a search-based check for the negative assertion is the correct approach.
• t.Cleanup in createTestPlugin and t.Setenv throughout keeps teardown automatic and test-scoped.
• All os.ReadFile calls on t.TempDir()-derived paths are correctly annotated with // #nosec G304.
• Proxy and TLS tests correctly use t.Skip when the required environment is absent.

[Udaysonu]

Critical — Security: "pwn request" vulnerability

pull_request_target runs with repository write permissions and access to secrets (e.g. RTLIC). This step checks out the PR contributor's HEAD code (github.event.pull_request.head.sha), meaning any PR author can embed a payload that exfiltrates secrets when a maintainer applies the safe to test label.

Fix options:

1. Only checkout github.ref (the base branch) under pull_request_target — never the PR's HEAD.
2. Replace pull_request_target with pull_request, which has no write permissions and no access to secrets, and add the safe to test label guard only if needed for external fork isolation.

[Udaysonu]

Minor — Unpinned @main action references

jfrog/.github/actions/install-go-with-cache@main (and install-local-artifactory@main two lines below) use mutable refs. A breaking push to the main branch of that repo will silently break this workflow without any PR or diff. Pin to a versioned tag or a commit SHA for reproducibility.

[Udaysonu]

Nit — --timeout 0 disables test timeout entirely

If a test deadlocks or hangs on a network call, this job runs forever and requires manual cancellation. A generous but finite value (e.g. --timeout 2h) ensures the runner recovers automatically.

[Udaysonu]

Major — Use require.NoError instead of assert.NoError for CopyDir

If biutils.CopyDir fails (missing fixture, permission error, etc.), the test continues with a partially-written pluginPath and fails at an unrelated assertion with a confusing message. require.NoError halts the test immediately at the failure site.

// Change:
assert.NoError(t, biutils.CopyDir(pluginSrc, pluginPath, true, nil))
// To:
require.NoError(t, biutils.CopyDir(pluginSrc, pluginPath, true, nil))

[Udaysonu]

Minor — Misleading comment: GetItemProps does not return nil on 404

GetItemProps returns an error on 404, not nil. The comment's stated justification for using the search-based check is incorrect. Please update the comment to accurately describe why a search is preferred (e.g. "a search-based check gives a cleaner absence assertion without relying on error message text").

[Udaysonu]

Minor — Fake zip content may cause unexpected test failure

[]byte("PK\x03\x04fake-zip-content") is not a valid zip archive — only the 4-byte magic bytes are present, the local file header structure is incomplete. If the publish command validates zip integrity before upload, this test will fail with a zip-format error rather than testing the prebuilt path.

Consider generating a minimal valid zip:

var buf bytes.Buffer
zw := zip.NewWriter(&buf)
require.NoError(t, zw.Close())
require.NoError(t, os.WriteFile(zipPath, buf.Bytes(), 0644))

[Udaysonu]

Minor — Test is non-assertive when the command succeeds

The if err != nil { ... } structure means: if evidence enforcement is not active on this Artifactory instance, the test passes trivially and validates nothing about the evidence gate. The scenario under test is silently skipped without any indication.

Consider making the bypass explicit:

if err != nil {
    assert.True(t, strings.Contains(err.Error(), "JFROG_AGENT_PLUGINS_DISABLE_QUIET_FAILURE") || ...)
} else {
    t.Log("evidence gate not enforced on this Artifactory instance; failure path not exercised")
}

[Udaysonu]

Major — Dead variable; test covers scenario already tested elsewhere

bogusServerURL is declared and immediately discarded with _ = bogusServerURL. The comment says it is "used conceptually," but the actual test only exercises --server-id=nonexistent-server-id-xyz — which is identical to TestAgentPluginsServerIDUnknown. Scenario #79 (actual network unreachability: DNS failure, TCP timeout) is not exercised.

Recommended fix: Either remove this test (it duplicates TestAgentPluginsServerIDUnknown), or implement it correctly — e.g. register a JFrog CLI server profile pointing to bogusServerURL and run the command against that registered profile to trigger a real network error.