fix(helm): resolve OCI build-info paths for non-root subpaths

[vjda]

• All tests passed. If this feature is not already covered by the tests, I added new tests.
• All static analysis checks passed.
• Appropriate label is added to auto generate release notes.
• I used gofmt for formatting the code before submitting the pull request.
• PR description is clear and concise, and it includes the proposed solution/fix.

---

Fixes #424

Summary

This PR fixes the same underlying OCI storage-path bug in the two affected Helm
OCI flows:

• jf helm push post-push build-info collection
• OCI dependency layer resolution in addOCILayersForDependency

In both flows, the bug is not that the OCI artifact is missing. The artifact is
already in Artifactory. The failure happens later, when the CLI resolves the
follow-up Artifactory path as if the chart always lived at the repository root
under:

<chart-name>/<chart-version>

That assumption breaks for valid OCI layouts such as:

• oci://helm-repo.artifactory.example.com/team-a/charts
• oci://artifactory.example.com/helm-repo/team-a/charts

In those cases, the real Artifactory storage path is:

team-a/charts/<chart-name>/<chart-version>

so the follow-up lookup misses the manifest/layers even though the artifacts
exist.

For dependencies, the same pattern appears because:

• dep.Id only contains chart-name:chart-version
• the missing OCI subpath lives in dep.Repository
• the old code looked up chart-name/chart-version at repo root instead of the
  real subpath/chart-name/chart-version

Why the bug happens

There are three important details here:

1. ORAS parsing is useful, but only for OCI syntax parsing.
   ORAS was already available in the package as a parser for OCI reference
   structure, but the affected flows were still resolving follow-up Artifactory
   paths with root-only assumptions.

   In other words, ORAS can tell us the OCI Registry and Repository
   components of a reference, but it does not by itself tell us semantically
   which part is the Artifactory repo key for every valid Artifactory access
   layout.

2. Any effective OCI URL that contains a path is potentially ambiguous from
   the URL alone.
   In other words, once the URL has path segments, the CLI cannot rely on a
   single local heuristic and pretend that the repo key is always obvious.

3. Real Artifactory AQL behavior matters here.
   During validation against a real Artifactory instance, wrong repo/path
   combinations returned a normal miss (200 + empty results), while real
   failures (for example auth problems) surfaced as operational errors.

   That matters because it justifies the final control flow:

   • empty results mean "this candidate did not match"
   • search errors mean "the search itself failed"

   That allowed the implementation to stay aligned with actual server behavior
   instead of introducing a speculative state machine around candidate search
   failures.

For this bug, the safest fix is therefore not "invent a stronger URL-only
heuristic", but to validate the small set of plausible candidates by searching
for the actual OCI artifacts that the flow needs to resolve.

Proposed Fix

This PR keeps the change intentionally small and OCI-only, but it covers the
two broken OCI call sites that matter here.

Instead of reopening the broader shared repository-resolution logic, the fix is
scoped to:

• push.go for post-push build-info resolution
• layers.go for OCI dependency layer resolution

The implementation follows the same small pattern in both places:

1. parse the OCI reference with ORAS to obtain reliable OCI syntax components
2. treat host-only OCI URLs as the only locally deterministic case
3. for OCI URLs with path, generate a small set of plausible Artifactory repo
   candidates locally inside the affected OCI flow
4. validate those candidates by searching for the actual OCI artifacts needed
   by that flow
5. once the winning candidate is found, propagate the real Artifactory paths to:
   • artifact lookup (subpath/chart/version)
   • manifest resolution
   • build-properties application on the manifest folder (push)
   • OCI dependency layer extraction (dependencies)

Why this approach

This approach keeps the fix correct, local, and easy to review.

1. Why not introduce a broader shared repository resolver here?

Because this bug is ultimately about two concrete OCI follow-up flows, not about
redesigning all repository resolution across Helm.

A broader shared resolver would increase the review surface and the regression
risk for code paths that are not required to fix this bug.

This PR is intentionally narrower:

• it fixes the two real OCI call sites that share the same root-only assumption
• it keeps the review surface small
• it avoids changing unrelated call sites and semantics
• it leaves Classic Helm untouched

That makes the change easier to reason about and much safer to review upstream.

2. Why not rely on ORAS alone?

Because ORAS is the right tool for OCI syntax parsing, but not for deciding
the Artifactory repo key in every valid Artifactory URL layout.

For example, ORAS can parse Registry and Repository, but if the effective
OCI URL contains path segments, that still does not prove which part is the
Artifactory repo key and which part is the subpath.

So the final rule in this PR is:

• ORAS parses structure
• real artifact search confirms semantics

That keeps the fix robust without pretending the URL alone is always enough.

3. Why validate using artifact search instead of repository metadata?

Because searching the actual OCI artifacts is the strongest scoped source of
truth for these flows.

Checking only whether a repo exists would still allow false positives:

• a plausible candidate repo might exist
• but the pushed chart may not actually live there at the required subpath

By validating with the same artifact lookup the flow already needs, the winning
candidate is proven against the concrete pushed chart location, not only against
repo metadata.

4. Why keep the change local to the affected OCI flows instead of a shared resolver?

Because the reported bugs are about concrete OCI follow-up flows, not about a
generic cross-domain repository resolver.

The final scope deliberately stays here:

• push.go for OCI post-push build-info
• layers.go for OCI dependency layer resolution

And deliberately stays out of here:

• Classic Helm behavior
• helm pull
• any broad shared-resolver redesign

That keeps the PR:

• focused
• reviewable
• low risk

Implementation details

• push.go resolves OCI push repo/subpath through a push-local helper:
  resolveOCIPushArtifacts(...)
• layers.go resolves OCI dependency repo/subpath locally inside
  addOCILayersForDependency(...)
• both flows use ORAS only for parsing and validate candidate repo/subpath
  combinations by searching for the real OCI artifacts
• searchPushedArtifacts(...) receives the resolved Artifactory storagePath
  directly instead of recomposing a root-only chart/version
• OCI dependency AQL is built with the real resolved storagePath instead of
  assuming repo root
• manifest-folder property application in push uses the real resolved
  Artifactory location
• the follow-up cleanup also:
  • removed obsolete buildProperties dead code from searchPushedArtifacts
  • normalized trailing slashes in OCI push URLs
  • introduced newManifestFolderReader(...) so the build-properties flow keeps
    a dedicated synthetic folder reader instead of reusing/mutating the search
    reader
  • moved the small OCI candidate primitive into repository.go
    (ociRepoCandidate + generateRepoCandidates) because it is now shared by
    both OCI flows, without turning it into a broad shared resolver
  • simplified candidate error handling after validating real AQL behavior
    against Artifactory: empty results are normal misses; search errors remain
    operational failures

Behavior preservation

This PR deliberately preserves the existing overall behavior outside the bug fix:

• native Helm push behavior is unchanged
• the push fix only affects the post-push build-info collection path
• the dependency fix only affects OCI dependency layer resolution
• no changes were made to Classic Helm resolution semantics
• no changes were made to helm pull
• no broad shared resolver was introduced

In other words, this is a targeted OCI correctness fix, not a generalized
resolver rewrite.

Test Coverage

Added/updated tests cover:

• OCI push target form 1: repo in host, no path
• OCI push target form 2: repo in host, with subpath
• OCI push target form 3: repo in path, no extra subpath
• OCI push target form 4: repo in path, with extra subpath
• ambiguous path cases where a plausible candidate repo exists but does not
  contain the pushed artifact
• host-based repo keys without -
• trailing-slash normalization for host-only URLs
• real manifest-folder path generation
• handlePushCommand integration-style coverage for forms 1–4
• OCI dependency with non-root subpath resolved through validated candidates
• OCI dependency root-only non-regression
• OCI dependency no-match behavior
• Classic Helm dependency path left untouched
• OCI candidate-generation helper coverage in repository_test.go

Validation run locally:

• go test -v -race ./artifactory/commands/helm -run '^TestHandlePushCommandResolvesOCIPaths$'
• go test -v -race ./artifactory/commands/helm
• go test -v -race ./artifactory/commands/helm -run '^(TestAddOCILayersForDependency|TestUpdateClassicHelmDependencyChecksumsLeavesExistingChecksumsUntouched|TestGenerateOCIRepoCandidates)$'

Manual Validation

In addition to the automated test coverage in this PR, I also validated the fix
manually against a real remote Artifactory instance hosted on jfrog.io.

Validation scope:

• exercised the real jf helm push flow against a remote OCI repository
• validated a non-root OCI subpath layout end-to-end
• confirmed that build-info collection resolved the correct Artifactory path
  after push
• exercised the real jf helm dependency build flow against an OCI dependency
  layout that also uses non-root subpaths
• confirmed that OCI dependency layer resolution followed the resolved
  Artifactory storage path instead of assuming a root-only chart/version
  layout

I also prepared local debug-only harnesses around the real package entrypoint
(HelmCommand.Run()), so both the push and dependency behaviors can be
reproduced and debugged locally without mixing environment-specific integration
code into the upstream PR.

I am not including those local integration/debug harnesses as part of the upstream
change because they depend on environment-specific credentials and infrastructure.

For reference, here are representative self-contained snippets for both
entrypoints. They are intentionally concise and use the real debug env var names
so they can be copied with minimal editing.

Snippets of the local debug-only harnesses:

//go:build debug

import (
    "os"
    "path/filepath"
    "testing"

    buildUtils "github.com/jfrog/jfrog-cli-core/v2/common/build"
    "github.com/jfrog/jfrog-cli-core/v2/utils/config"
)

func TestDebugHelmPushEntryPoint(t *testing.T) {
    chartPath := os.Getenv("HELM_TEST_DEBUG_CHART_PATH")
    registryURL := os.Getenv("HELM_TEST_DEBUG_REGISTRY_URL")
    artifactoryURL := os.Getenv("HELM_TEST_DEBUG_ARTIFACTORY_URL")
    buildName := os.Getenv("HELM_TEST_DEBUG_BUILD_NAME")
    buildNumber := os.Getenv("HELM_TEST_DEBUG_BUILD_NUMBER")
    project := os.Getenv("HELM_TEST_DEBUG_PROJECT")

    if chartPath == "" || registryURL == "" || artifactoryURL == "" {
        t.Skip("local debug-only test")
    }

    if buildName == "" {
        buildName = "debug-helm-push"
    }
    if buildNumber == "" {
        buildNumber = "1"
    }

    buildConfig := buildUtils.NewBuildConfiguration(
        buildName,
        buildNumber,
        "",
        project,
    )

    cmd := NewHelmCommand().
        SetHelmCmdName("push").
        SetHelmArgs([]string{chartPath, registryURL}).
        SetWorkingDirectory(filepath.Dir(chartPath)).
        SetServerDetails(&config.ServerDetails{
            ArtifactoryUrl: artifactoryURL,
            AccessToken:    os.Getenv("HELM_TEST_DEBUG_ACCESS_TOKEN"),
            User:           os.Getenv("HELM_TEST_DEBUG_USER"),
            Password:       os.Getenv("HELM_TEST_DEBUG_PASSWORD"),
        }).
        SetBuildConfiguration(buildConfig)

    if err := cmd.Run(); err != nil {
        t.Fatalf("debug helm push entrypoint failed: %v", err)
    }
}

func TestDebugHelmDependenciesEntryPoint(t *testing.T) {
    workingDir := os.Getenv("HELM_TEST_DEBUG_WORKING_DIR")
    artifactoryURL := os.Getenv("HELM_TEST_DEBUG_ARTIFACTORY_URL")
    buildName := os.Getenv("HELM_TEST_DEBUG_BUILD_NAME")
    buildNumber := os.Getenv("HELM_TEST_DEBUG_BUILD_NUMBER")
    project := os.Getenv("HELM_TEST_DEBUG_PROJECT")

    // workingDir should point to the chart project whose OCI dependencies
    // will be resolved by `helm dependency build`.
    if workingDir == "" || artifactoryURL == "" {
        t.Skip("local debug-only test")
    }

    if buildName == "" {
        buildName = "debug-helm-dependency"
    }
    if buildNumber == "" {
        buildNumber = "1"
    }

    buildConfig := buildUtils.NewBuildConfiguration(
        buildName,
        buildNumber,
        "",
        project,
    )

    cmd := NewHelmCommand().
        SetHelmCmdName("dependency").
        SetHelmArgs([]string{"build"}).
        SetWorkingDirectory(workingDir).
        SetServerDetails(&config.ServerDetails{
            ArtifactoryUrl: artifactoryURL,
            AccessToken:    os.Getenv("HELM_TEST_DEBUG_ACCESS_TOKEN"),
            User:           os.Getenv("HELM_TEST_DEBUG_USER"),
            Password:       os.Getenv("HELM_TEST_DEBUG_PASSWORD"),
        }).
        SetBuildConfiguration(buildConfig)

    if err := cmd.Run(); err != nil {
        t.Fatalf("debug helm dependency entrypoint failed: %v", err)
    }
}

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[vjda]

I have read the CLA Document and I hereby sign the CLA

[Copilot]

Pull request overview

This PR fixes Helm OCI follow-up Artifactory path resolution when charts/dependencies are stored under non-root subpaths, by generating plausible repo/subpath candidates and validating them via artifact searches before proceeding.

Changes:

• Add a shared OCI repo-candidate primitive (ociRepoCandidate) and candidate generation helper.
• Update OCI push build-info collection to resolve the correct repo key + storage path (including subpaths) before manifest/layer lookup and property application.
• Update OCI dependency layer resolution to use validated repo/subpath candidates instead of assuming <chart>/<version> at repo root, and add focused unit tests for both flows.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
artifactory/commands/helm/repository.go	Introduces OCI repo/subpath candidate structure and generator shared by push/dependency flows.
artifactory/commands/helm/repository_test.go	Adds unit tests for OCI candidate generation behavior.
artifactory/commands/helm/push.go	Resolves OCI push artifacts using validated candidates and applies build props to the resolved manifest folder path.
artifactory/commands/helm/push_test.go	Adds comprehensive tests for OCI push resolution forms and manifest-folder property application behavior.
artifactory/commands/helm/layers.go	Updates OCI dependency AQL lookup to use resolved storage paths (including subpaths) via validated candidates.
artifactory/commands/helm/layers_test.go	Adds tests covering non-root OCI dependencies, root-only non-regression, and no-match behavior.

Comments suppressed due to low confidence (1)

artifactory/commands/helm/push.go:234

• SetProps errors are currently ignored (both the status code and error are discarded). This can cause the push build-info flow to succeed while build properties were never applied. Please propagate/log the SetProps error (and ideally make applyBuildPropertiesOnManifestFolder return it) so failures are surfaced correctly.

func addBuildPropertiesOnArtifacts(serviceManager artifactory.ArtifactoryServicesManager, reader *content.ContentReader, buildProps string) {
	propsParams := services.PropsParams{
		Reader:      reader,
		Props:       buildProps,
		IsRecursive: true,
	}
	_, _ = serviceManager.SetProps(propsParams)
}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[vjda]

Addressed all of copilot review comments in a small follow-up commit.

• preserved the original getManifest error context with %w
• made the host-only miss path report "no OCI artifacts found" instead of a
  misleading repo-key resolution error
• updated the stale overwriteReaderWithManifestFolder comment
• included parseErr in the dependency-side log path
• cleaned the push_test.go formatting issue and re-ran gofmt

Focused tests were re-run after the changes.

[agrasth]

For ambiguous URLs like oci://helm-repo.art.com/team-a/charts, the code tries:

1. First: team-a repo with subpath charts/
2. Second: helm-repo repo with subpath team-a/charts/

In typical JFrog setups, virtual host repos (e.g., helm-repo.art.com → helm-repo repo) are often more common than repos named after subdirectories. Would reversing the order reduce unnecessary AQL queries in practice?

[vjda]

Thanks for calling this out. I updated the candidate order to try the host-derived OCI repo first and kept the path-based candidate as the fallback.

I don't think this makes the ambiguous cases universally "correct" in a strict sense, since the current flow still resolves on the first successful artifact lookup. So, as I see it, the ordering is effectively a policy choice when both layouts could be plausible.

The reason for this update is practical: Artifactory commonly uses the host-based OCI layout, so preferring that candidate first aligns better with the more typical setup and can avoid an unnecessary AQL query in that case.

I also updated the focused push/dependency tests so they explicitly document the new host-first behavior and its fallback semantics.

[vjda]

PR #430 has now been merged and appears to cover the same issue space.

I want to leave one point clearly on the record:

if this work was already being handled internally, then reviewing this PR, requesting changes on it, and later merging the same solution path through a different internal branch — without linking it back here or leaving any traceability note — is not a good way to manage external contributions.

From the contributor side, the experience was:

• this PR was reviewed in detail
• I spent time addressing the requested changes
• the PR was then approved
• in parallel, a separate PR covering the same problem space was opened and merged
• I only discovered that by investigating the conflict notification myself

That creates avoidable wasted effort and leaves the external contributor without basic visibility into what happened.

To make things more confusing, the repository’s CONTRIBUTING.md currently says that pull requests should target the dev branch, while this repository does not even have a dev branch. So from the outside, both the documented contribution flow and the actual review flow are unclear.

I’m not raising this to argue about ownership of the final code, but to highlight a process problem:
if contributions may be superseded by an internal branch, that should be communicated explicitly and as early as possible, with proper linkage between the PRs.

Otherwise, the repository is effectively asking external contributors to spend time on work that may already be decided elsewhere, without any transparency.

Please improve the contributor flow here:
either pause/close superseded external PRs early, or explicitly link and explain when the work is being continued internally.