RTECO-1536 - Add native RubyGems/Bundler command with auth and build-info - #499
Open
agrasth wants to merge 27 commits into
Open
RTECO-1536 - Add native RubyGems/Bundler command with auth and build-info#499agrasth wants to merge 27 commits into
agrasth wants to merge 27 commits into
Conversation
Implements the core business logic for `jf ruby gem|bundle` native command:
- RubyCommand dispatcher: routes to gem or bundle with stdio passthrough
- Authentication injection:
- Bundler: BUNDLE_<HOST> env var with proper host-key normalization
- gem: GEM_HOST_API_KEY env var
- Non-destructive, per-process only, respects existing native credentials
- Host-match safety: skips injection when source host differs from server
- Help bypass: no auth for help/-h/--help requests
- Repository discovery: --source/--host args > Gemfile source > gem sources
- Build-info collection:
- Dependencies: via RubygemsFlexPack for bundle install/update/lock/add
and opportunistic gem install/fetch
- Artifacts: gem build (local .gem checksums) and gem push (+ property tagging)
- AQL checksum enrichment: single batched query for all deps
- 10 unit tests covering auth, repo extraction, help detection, host matching
Note: go.mod contains local replace directive for build-info-go (development only,
to be replaced with proper version bump at merge time).
Co-authored-by: Cursor <cursoragent@cursor.com>
agrasth
requested review from
a team,
bhanurp,
fluxxBot,
itsmeleela,
naveenku-jfrog and
udaykb2
June 29, 2026 04:07
3 tasks
agrasth
changed the title
Bug fixes: - Bug 1: GEM_HOST_API_KEY now uses Basic base64 encoding (was raw user:pass which Artifactory rejected) - Bug 2: gem install/fetch embeds credentials in --source URL for index downloads (specs.4.8.gz) since GEM_HOST_API_KEY is not used for those requests in RubyGems 3.x - Bug 3: explicit no-args error before help bypass prevents silent fallthrough to gem help - Bug 4: skip credential injection when no Artifactory source is discovered (even with --server-id) New feature: - --repo flag constructs the full Artifactory gems API URL from server config + repo name, eliminating the need to pass full URLs. For gem install/push, injects --source/--host automatically. Also enriches dependency repo path from AQL results in build-info. Co-authored-by: Cursor <cursoragent@cursor.com>
Scope classification: - New parseGemfileGroups() parses Gemfile group blocks and inline groups - Classifies gems as production/development/test - Transitive deps inherit scopes from their parents - Scopes passed to build-info for Xray scanning jf setup integration: - Added project.Ruby to packageManagerToRepositoryPackageType (Gems) - New configureRuby() method configures Bundler credentials via 'bundle config set' and adds source to ~/.gemrc - Ruby now appears in 'jf setup' supported package managers list Co-authored-by: Cursor <cursoragent@cursor.com>
…rovements - Remove gem build from artifact collection (local-only, no Artifactory path) - Only gem push now records artifacts in build-info - Implement hybrid checksum strategy: local gem cache first, AQL fallback - Add gem push auth via temporary ~/.gem/credentials (all RubyGems versions) - Embed credentials in --source URL for gem install/fetch index downloads - Strip trailing slash from --host to prevent 405 double-slash issue - Skip auth injection entirely for gem build (pure local operation) - Remove dead parseGemBuildOutputFile function and its tests Co-authored-by: Cursor <cursoragent@cursor.com>
bundle lock only resolves the dependency graph and writes Gemfile.lock without downloading any gems. No actual consumption from Artifactory happens, so recording dependencies in build-info is incorrect. Co-authored-by: Cursor <cursoragent@cursor.com>
Points to feature branch commit instead of local path so CI can resolve the dependency without a local checkout. Co-authored-by: Cursor <cursoragent@cursor.com>
Artifactory accepts basic auth with an empty username when the password is a valid access token (reference or JWT). This matches the Go module proxy pattern. Changed credential check from requiring both user+pass to only requiring pass (token). Co-authored-by: Cursor <cursoragent@cursor.com>
Previously when --server-id pointed to a non-existent config entry, the command logged a warning and fell through to rubygems.org. Now it returns a clear error, matching user intent. Co-authored-by: Cursor <cursoragent@cursor.com>
Bundler credential lookup may check only the hostname key (e.g., BUNDLE_LOCALHOST) ignoring the port. Now inject credentials under both the host:port key and hostname-only key to cover all versions. Co-authored-by: Cursor <cursoragent@cursor.com>
…f setup ruby `bundle config set <host> <user:pass>` silently misparses on Bundler 1.x (no `set` subcommand there), writing a garbage BUNDLE_SET key while reporting success. ~/.gemrc writes also relied on fragile substring matching that couldn't dedupe across different repos. configureRuby now reads/writes ~/.bundle/config and ~/.gemrc as YAML directly (mirroring cargo/setup.go's TOML approach), reusing ruby.BundleEnvKeyForHost (now exported) for host-key normalization so setup-time and runtime-injection keys can't drift apart. A write failure is now a real error instead of a silently-ignored warning. Re-configuring a different repo keeps both sources in ~/.gemrc, most recent first, since gem install natively searches every listed source.
|
Warning Review limit reached
Next review available in: 3 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughNative RubyGems and Bundler execution now supports repository discovery, host-safe authentication, command execution, and optional build-info collection. Ruby setup writes Bundler and RubyGems YAML configuration with source ordering, credential rotation, preservation, and error handling. Comprehensive tests cover these flows. ChangesRuby integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant RubyCommand
participant Artifactory
participant RubyTool
RubyCommand->>Artifactory: resolve repository and credentials
RubyCommand->>RubyTool: run native gem or bundle command
RubyTool->>Artifactory: read or publish gem data
RubyTool-->>RubyCommand: return output for build-info collection
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
artifactory/commands/ruby/ruby.go (1)
68-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider caching the resolved server details.
ServerDetails()re-reads the config on every call since the resolved value isn't stored back intorc.serverDetails. Currently only called once fromRun(), but caching makes it safe for repeated use.♻️ Proposed change
func (rc *RubyCommand) ServerDetails() (*config.ServerDetails, error) { if rc.serverDetails != nil { return rc.serverDetails, nil } - return rubyResolveServerDetails(rc.serverID) + details, err := rubyResolveServerDetails(rc.serverID) + if err != nil { + return nil, err + } + rc.serverDetails = details + return details, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/ruby/ruby.go` around lines 68 - 73, Update RubyCommand.ServerDetails to store the result of rubyResolveServerDetails in rc.serverDetails before returning it, while preserving the existing cached fast path and error propagation.artifactory/commands/ruby/native_ruby.go (2)
488-490: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
userparameter is unused.
rubyHasCredentialsonly checkspass; theuserargument is dead weight and will be flagged byrevive/unparam.♻️ Proposed change
-// rubyHasCredentials returns true when at least a password or token is available. -func rubyHasCredentials(user, pass string) bool { - return pass != "" -} +// rubyHasCredentials returns true when at least a password or token is available. +func rubyHasCredentials(pass string) bool { + return pass != "" +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/ruby/native_ruby.go` around lines 488 - 490, Update rubyHasCredentials to remove the unused user parameter and retain only the pass argument, then update every call site to match the simplified signature while preserving its existing password-present check.
194-230: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWire in the documented
rubyEmbedCredsInHostArgfallback or remove it.
rubyEmbedCredsInHostArghas no call sites outside tests, whileRunonly strips--hostURLs and writes~/.gem/credentialsforgem push. The helper’s own comment says it’s the fallback for RubyGems ≤ 3.0.x who don’t useGEM_HOST_API_KEY, but nothing applies it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/ruby/native_ruby.go` around lines 194 - 230, Integrate rubyEmbedCredsInHostArg into the Run flow for RubyGems versions that do not use GEM_HOST_API_KEY, applying it to the command arguments before execution; alternatively remove the unused helper and its tests/comment if the fallback is no longer supported. Preserve the existing --host handling and credential-file behavior for supported RubyGems versions.artifactory/commands/ruby/native_ruby_test.go (1)
576-578: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCheck setup errors in the fixture.
os.MkdirAll/os.WriteFileerrors are discarded; a failed fixture would surface as a confusing assertion failure later.💚 Proposed change
- os.MkdirAll(gemDir, 0700) existingContent := "---\n:rubygems_api_key: existing-key\n" - os.WriteFile(filepath.Join(gemDir, "credentials"), []byte(existingContent), 0600) + require.NoError(t, os.MkdirAll(gemDir, 0700)) + require.NoError(t, os.WriteFile(filepath.Join(gemDir, "credentials"), []byte(existingContent), 0600))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/ruby/native_ruby_test.go` around lines 576 - 578, Check and handle the errors returned by os.MkdirAll and os.WriteFile in the fixture setup before continuing, using the test’s existing failure mechanism so setup failures are reported immediately rather than during later assertions.artifactory/commands/setup/setup.go (1)
626-632: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
filepath.Joinfor home-relative paths.String concatenation with
/is inconsistent with the rest of this file's path handling and non-idiomatic on Windows.♻️ Proposed change
- bundleDir := home + "/.bundle" - configPath := bundleDir + "/config" + bundleDir := filepath.Join(home, ".bundle") + configPath := filepath.Join(bundleDir, "config")- gemrcPath := home + "/.gemrc" + gemrcPath := filepath.Join(home, ".gemrc")Also applies to: 662-667
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/setup/setup.go` around lines 626 - 632, Update writeBundleConfig to construct bundleDir and configPath with filepath.Join instead of slash-based string concatenation, preserving the existing ~/.bundle/config locations and platform-independent path handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@artifactory/commands/ruby/native_ruby_test.go`:
- Around line 509-512: Update the affected tests around the HOME setup to use
t.Setenv instead of os.Setenv with deferred restoration, and override the
Windows-specific USERPROFILE variable as well as HOME using the temporary
directory. Apply this consistently to all three referenced tests so
os.UserHomeDir resolves to the isolated temporary home on every platform.
In `@artifactory/commands/ruby/native_ruby.go`:
- Around line 150-189: Remove credential embedding from
rubyEmbedCredsInSourceArg and route RubyGems authentication through the existing
rubyWriteTempGemCredentials file-based mechanism or a supported
config-file/environment path instead. Ensure install/fetch commands use that
authentication path without placing user, password, or tokens in argv, and
redact any source URL before logging.
- Around line 1045-1051: Update the artifact assembly in rubySetBuildProperties
so Artifact.Path contains the repository-relative Artifactory path rather than
filepath.Base(path). Reuse the existing AQL lookup for the matching gem artifact
name to obtain its actual repository and path before appending the
buildinfo.Artifact, preserving the <repo>/gems/<file>.gem location.
- Around line 1146-1155: Update the group-block parsing around currentGroups and
the `group`/`end` detection to track nested `do ... end` depth, rather than
clearing the group on the first closing `end`. Increment depth for nested
blocks, decrement on each matching `end`, and clear currentGroups only when the
outer group block closes so subsequent gems remain classified correctly.
- Around line 85-93: Update the native Ruby authentication branches around
rubyEmbedCredsInSourceArg and the push credential-writing logic to apply the
same configured-host validation enforced by injectAuth. Only embed credentials
in --source URLs or write ~/.gem/credentials when the source host matches the
configured Artifactory host, while preserving existing behavior for valid
matching hosts.
In `@artifactory/commands/setup/setup_test.go`:
- Around line 949-960: Update the writeBundleConfig test to create the existing
Bundler config with mode 0644, then assert its mode is 0600 after
writeBundleConfig completes. Ensure writeBundleConfig itself enforces 0600 for
existing files, using chmod or atomic replacement as needed, while preserving
the existing config contents and entries.
In `@artifactory/commands/setup/setup.go`:
- Around line 681-694: Update the :sources handling before calling
reorderGemrcSources so malformed or empty values, including nil, maps, or an
empty list, fall back to rubygemsDefaultSource. Preserve valid configured source
entries and ensure the rewritten configuration always retains the RubyGems
default when no usable sources are present.
In `@go.mod`:
- Line 201: The go.mod entry for github.com/jfrog/build-info-go currently uses a
replace directive; remove that replacement and add or update the direct require
entry to v1.13.1-0.20260715194847-6e04c9b133c8 so flexpack imports resolve
through the declared module requirement.
---
Nitpick comments:
In `@artifactory/commands/ruby/native_ruby_test.go`:
- Around line 576-578: Check and handle the errors returned by os.MkdirAll and
os.WriteFile in the fixture setup before continuing, using the test’s existing
failure mechanism so setup failures are reported immediately rather than during
later assertions.
In `@artifactory/commands/ruby/native_ruby.go`:
- Around line 488-490: Update rubyHasCredentials to remove the unused user
parameter and retain only the pass argument, then update every call site to
match the simplified signature while preserving its existing password-present
check.
- Around line 194-230: Integrate rubyEmbedCredsInHostArg into the Run flow for
RubyGems versions that do not use GEM_HOST_API_KEY, applying it to the command
arguments before execution; alternatively remove the unused helper and its
tests/comment if the fallback is no longer supported. Preserve the existing
--host handling and credential-file behavior for supported RubyGems versions.
In `@artifactory/commands/ruby/ruby.go`:
- Around line 68-73: Update RubyCommand.ServerDetails to store the result of
rubyResolveServerDetails in rc.serverDetails before returning it, while
preserving the existing cached fast path and error propagation.
In `@artifactory/commands/setup/setup.go`:
- Around line 626-632: Update writeBundleConfig to construct bundleDir and
configPath with filepath.Join instead of slash-based string concatenation,
preserving the existing ~/.bundle/config locations and platform-independent path
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 263a8b8e-8c14-4519-8343-22b0a31f7fe7
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
artifactory/commands/ruby/native_ruby.goartifactory/commands/ruby/native_ruby_test.goartifactory/commands/ruby/ruby.goartifactory/commands/setup/setup.goartifactory/commands/setup/setup_test.godocs/superpowers/specs/2026-07-31-ruby-setup-fix-design.mdgo.mod
Brings `jf setup ruby` to parity with cargo/nuget/apt, where setup alone is enough for the native tool to resolve and authenticate with no manifest edit. Verified end-to-end against a private Artifactory repo with real Bundler 1.17.2 and RubyGems. Bundler mirror, so no Gemfile edit is needed ~/.bundle/config now gets a `mirror.https://rubygems.org` entry pointing at the Artifactory repository, the Bundler analogue of Cargo's `[source.crates-io] replace-with`. An unmodified `source "https://rubygems.org"` Gemfile resolves through Artifactory, and the lockfile still records rubygems.org, so it stays portable and does not leak the instance URL. Verified present in Bundler 1.17 through 4.0; fallback_timeout defaults to 0, so no TCP probe runs. Fix: bare `gem install` could not authenticate (401) RubyGems has no credential store for installs, so a credential-free source in ~/.gemrc meant every install against a private repo failed with 401. The ~/.gemrc source now embeds credentials, and the file is written 0600. Entries are matched with credentials stripped, so rotating a token replaces the stale entry instead of leaving `gem install` retrying a dead credential. Fix: credential key was wrong for dashed and ported hosts Bundler's key normalization changed between majors: 1.x keeps dashes, 2.x+ turns them into "___". We only ever wrote one spelling, so a dashed host such as my-company.jfrog.io silently failed to authenticate on one of them. BundleCredentialKeys now emits every spelling Bundler may look under, and is shared by setup and the runtime injection in `jf ruby bundle` so the two cannot drift. Setup also keys on the hostname rather than host:port, because Bundler's fallback uses uri.host, which excludes the port; the previous host:port key was never read at all. Bundler parses ~/.bundle/config with its own line-based stub serializer rather than a real YAML parser, so tests now assert round-tripping through a port of that parser instead of merely checking the output is valid YAML.
The host-match guard that protects against sending Artifactory credentials to a third-party registry lived inside injectAuth and only short-circuited the environment variables. Run() then went on to embed credentials in argv and write ~/.gem/credentials regardless, so the guard was bypassed on exactly the two paths that persist a credential. jf ruby gem install foo --source https://gems.thirdparty.com/api/gems/x/ with no --server-id logged "skipping credential injection" and then passed --source https://user:<token>@gems.thirdparty.com/... to the child process. The gem push path did the same via ~/.gem/credentials. Authorization now lives in authorizedForSource and gates every credential path in Run(), with injectAuth keeping its own check as defence in depth. It is evaluated against the host the native tool will really contact, so `--repo <artifactory> --host <third-party>` can no longer authorize against one host and hand the credential to another. Also fixes `gem push` targeting the wrong host entirely: --host was only injected when --repo was passed, so a push whose source came from the Gemfile or ~/.gemrc fell back to RubyGems' default host and sent both the gem and the Artifactory credential to rubygems.org. Two build-info correctness fixes found in the same audit: - AQL enrichment read the checksum from the first matching result but the repository path from the last, so a gem with a platform-specific sibling (nokogiri-1.16.0.gem alongside nokogiri-1.16.0-arm64-darwin.gem) published a checksum for a file the recorded path did not point at. Both now come from one result, and an exact <name>-<version>.gem wins over a platform variant. - `gem install -v "~> 13.0"` recorded the requirement as the version, producing a dependency ID of "rake:~> 13.0" that matches no artifact. Requirements are now rejected so the installed version is queried instead.
The build-info-go version the RubyGems code needs was pinned with a replace directive, while the require line still named an older commit. Go ignores replace directives from dependencies, so a consumer resolved the stale require and failed to compile the ruby package with undefined flexpack.GemConfig and buildinfo.Gem. Promoting the pin to the require line makes it propagate through normal minimal version selection, so jfrog-cli no longer needs its own matching replace. The resolved module is byte-identical to what the replace already produced, so this does not change the build of this module.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
artifactory/commands/ruby/native_ruby.go (1)
470-496: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueVerify that colon-bearing Bundler keys work as environment variables.
BundleCredentialKeys("localhost:8081")returnsBUNDLE_LOCALHOST:8081. The code passes every candidate as a process environment variable name. Unix accepts:in a name, but Windows environment APIs and some shells do not, and Bundler reads the ported form primarily from~/.bundle/config. Confirm the behaviour on Windows, or filter candidates that are not valid environment variable names before appending them toextraEnv.♻️ Optional filter
for _, key := range candidates { if seen[key] { continue } seen[key] = true + if strings.ContainsAny(key, ":=") { + // Not a portable environment variable name; only usable as a ~/.bundle/config key. + continue + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/ruby/native_ruby.go` around lines 470 - 496, Update the Bundler credential injection in the nativeTool/ toolBundle branch to avoid passing colon-containing keys such as those produced by BundleCredentialKeys(host) as environment variable names. Validate or filter candidates before appending to extraEnv, while preserving valid bare-host keys and existing-credential handling; ensure Windows-safe environment variable behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@artifactory/commands/ruby/native_ruby.go`:
- Around line 470-496: Update the Bundler credential injection in the
nativeTool/ toolBundle branch to avoid passing colon-containing keys such as
those produced by BundleCredentialKeys(host) as environment variable names.
Validate or filter candidates before appending to extraEnv, while preserving
valid bare-host keys and existing-credential handling; ensure Windows-safe
environment variable behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ca7d43c0-5803-4709-8926-d8ff73ded3ce
📒 Files selected for processing (5)
artifactory/commands/ruby/native_ruby.goartifactory/commands/ruby/native_ruby_test.goartifactory/commands/setup/setup.goartifactory/commands/setup/setup_test.gogo.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- artifactory/commands/setup/setup.go
Three build-info reporting fixes, all found while inspecting published build-info against a live instance. Artifact path was the bare file name. RubyGems repositories nest gems under "gems/", so build-info reported a location the gem was never published to: "agrasthn-demo-gem-0.1.0.gem" instead of "gems/agrasthn-demo-gem-0.1.0.gem", and OriginalDeploymentRepo was never set. The path now comes from the same AQL search that already sets the build properties, so it cannot disagree with where the file actually landed. This matters because Artifactory persists an artifact's path, unlike a dependency's, which it drops on ingest. Failed dependency enrichment was logged at debug level, so publishing every dependency without a checksum looked exactly like success. It is now a warning that names the likely cause: AQL cannot search a virtual repository, and gems served from a local gem cache never reach Artifactory to be found. `gem build` accepted --build-name and --build-number and silently recorded nothing, because it is a local-only operation with no artifact in Artifactory yet. That is intentional, but it now says so and points at `gem push`, rather than leaving the flags looking accepted.
`jf setup ruby` kept https://rubygems.org at the front of ~/.gemrc's :sources: list. RubyGems queries sources in list order, so a plain `gem install` reached the public registry before Artifactory and setup had no practical effect on it. The public source is now removed, matching what the Bundler mirror already does and what setup does for npm and cargo, which replace the public registry rather than racing it. Artifactory repositories configured across separate runs still coexist, most recently configured first, because `gem install` genuinely does search several sources. This assumes the configured repository is virtual or remote-backed so it can still serve public gems, which is the same assumption every other package manager's setup makes. The four "Ruby auth [...]" messages are now debug rather than info. Which environment variable carries the credential, and whether GEM_HOST_API_KEY or a URL-embedded credential is used, is implementation detail that only matters when diagnosing an auth failure.
… identity Gemfile discovery only looked in the current directory, while Bundler honours $BUNDLE_GEMFILE and otherwise walks up until it finds one. Running any ruby command from a subdirectory of a project therefore found no source, injected no credentials, and failed with a bare "bundle install failed: exit status 16" that pointed at nothing. Discovery now matches Bundler, and both the source lookup and build-info collection are anchored on the resolved directory so Gemfile.lock is found too. Module IDs came from three unrelated sources: the working-directory base name for bundle and gem push, and the literal string "gem-" plus the subcommand for gem install. That produced IDs like "tmp.46NMMEmRLv" from a temporary directory, made every project in the world report module "gem-install", and split one project's dependencies and artifact across two modules whenever the build and the push ran from different directories. All three paths now share one helper preferring the gemspec's "<name>:<version>", which is the Ruby equivalent of package.json or pom.xml and matches how npm and Maven name modules. Applications with a Gemfile and no gemspec fall back to the Gemfile's directory name. A gemspec that computes its version in Ruby cannot be read without executing it, so those report the name alone rather than a wrong version. The identity is also passed to flexpack, which otherwise falls back to the working-directory name itself.
…rupts Five defects that all caused silent wrong behaviour rather than an error. The subcommand was read from args[0], so any global flag in front of it (`gem --backtrace install rake`) matched no case: no credentials injected, no source appended, no build-info collected, and still exit 0. It is now the first non-flag argument, skipping the values of the few global flags that take one. Injected --source/--host were appended after any "--" separator, where gem forwards them to the C extension build instead of consuming them itself, so `gem install nokogiri -- --use-system-libraries` resolved from the default source and passed a bogus argument to extconf. They now go before the separator. A block nested in a Gemfile group closed that group early, because any `end` cleared the group state. Every gem after an inner `platforms :ruby do ... end` was therefore scoped production instead of development, and the wrong scope propagated to its transitive dependencies. The publish hint dropped --project even when it was passed. Build partials are stored per project, so following the printed command published an empty build and looked like the build had been lost. Ctrl-C during `gem push` left the Artifactory token in ~/.gem/credentials, because cleanup was deferred and Go exits on SIGINT without running defers. Interrupts are now handled, cleanup runs once whichever path is taken, and the signal is re-raised so the exit status still reflects the interruption.
`gem build` accepted --build-name and --build-number and recorded nothing, so a project that arrives with its gems already vendored and is built and published without ever running an install reported no dependencies at all. Dependencies now come from Gemfile.lock, reusing the same collector the bundle commands use. The lock file is the only honest source for a resolved set: a gemspec or Gemfile states requirements such as "~> 13.0", which are not versions and would produce dependency IDs matching no artifact in Artifactory. With no lock file present there is nothing resolved to record, and that is now stated rather than silently accepted. Verified against a live instance that running an install and a build under the same build name merges by dependency ID rather than duplicating, so the install-then-build flow stays clean. The built .gem is still recorded only by `gem push`, which is the point it gains an Artifactory path.
…` works RubyGems resolves index files relative to the source URL, so a source without a trailing slash has its last path segment replaced: it requested /artifactory/api/gems/specs.4.8.gz, dropping the repository name entirely, and every plain `gem install` failed with "server did not return a valid file". That is the exact flow `jf setup ruby` exists to enable, and it was broken for any bare gem command. Earlier manual checks missed it because they passed an explicit --source, which jf appends already slash-terminated. Bundler normalises the trailing slash itself, so the same URL stays correct for the mirror entry and for the Gemfile line printed to the user. Source equality already ignores a trailing slash, so re-running setup still replaces rather than duplicates the entry.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
artifactory/commands/ruby/native_ruby.go (1)
1766-1796: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCheck
searchReader.GetError()after reading search records.
NextRecordcan return the last record and a subsequent read error; exiting the loop without callingGetError()lets failed searches skipartifact.Pathupdates and still reachSetPropsfromReset().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/ruby/native_ruby.go` around lines 1766 - 1796, The artifact search loop around NextRecord must check searchReader.GetError() after consuming records and before Reset(). Handle any read error so failed searches do not proceed to SetProps with incomplete artifact.Path data, while preserving the existing matching-record update behavior.
🧹 Nitpick comments (1)
artifactory/commands/ruby/native_ruby.go (1)
944-952: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSet
bundleInstalledPackages' cwd to the resolved Bundler directory.
bundleInstalledPackagescallsbundle listfromworkingDir, whileFlexPackandparseGemfileGroupsuserubyGemfileDir(workingDir). Running from a project subdirectory can makebundle listfail, causing build-info to fall back to the full lockfile instead of using installed-package metadata. Update the parser to callbundle listin the resolved Gemfile directory, or expose that helper’s logic throughbundleInstalledPackages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/ruby/native_ruby.go` around lines 944 - 952, The bundle-installed package parsing path must use the resolved Bundler directory rather than the original working directory. Update bundleInstalledPackages, or the parser invoking it, so bundle list runs with rubyGemfileDir(workingDir) as its cwd, matching collectLockfileDependencies and parseGemfileGroups.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@artifactory/commands/ruby/native_ruby.go`:
- Around line 1457-1472: The nested block counter only increments when the line
ends with "do", but other block-opening patterns like `platforms :mri do |p|`
(ending with `|`) or conditional keywords like `if`, `unless`, `case`, `begin`
also open blocks without the `do` suffix. When their corresponding `end`
statements are encountered, the nested counter is not decremented, causing
currentGroups to be incorrectly cleared and remaining gems to be misclassified
as production. Create package-level regex patterns to match block openers that
do not end with "do" (such as those ending with `|` or conditional keywords),
then update the condition that currently only checks `strings.HasSuffix(line,
"do")` to also use these patterns to detect all block openers and increment
nested consistently.
---
Outside diff comments:
In `@artifactory/commands/ruby/native_ruby.go`:
- Around line 1766-1796: The artifact search loop around NextRecord must check
searchReader.GetError() after consuming records and before Reset(). Handle any
read error so failed searches do not proceed to SetProps with incomplete
artifact.Path data, while preserving the existing matching-record update
behavior.
---
Nitpick comments:
In `@artifactory/commands/ruby/native_ruby.go`:
- Around line 944-952: The bundle-installed package parsing path must use the
resolved Bundler directory rather than the original working directory. Update
bundleInstalledPackages, or the parser invoking it, so bundle list runs with
rubyGemfileDir(workingDir) as its cwd, matching collectLockfileDependencies and
parseGemfileGroups.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c51f73df-de6a-47f4-9263-97c696b1571b
📒 Files selected for processing (4)
artifactory/commands/ruby/native_ruby.goartifactory/commands/ruby/native_ruby_test.goartifactory/commands/setup/setup.goartifactory/commands/setup/setup_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- artifactory/commands/setup/setup.go
- artifactory/commands/ruby/native_ruby_test.go
- artifactory/commands/setup/setup_test.go
Static Check and Go-Sec both failed. No behaviour changes here. errcheck: the temporary-home test helpers used os.Setenv with an unchecked error and a manual restore. Replaced with t.Setenv, which restores the value itself, and the remaining MkdirAll/WriteFile calls are now asserted. nilerr: a missing Gemfile.lock is an expected condition rather than a failure, so the check no longer binds an error it deliberately ignores. It reads through a small rubyFileExists helper instead. unparam: rubyHasCredentials only ever consulted the password, parseGemCommandOutput never used its subCommand argument, and rubyCollectGemArtifacts could not fail. Signatures trimmed to what they actually use. G703: the credentials path is built from the home directory, which comes from the environment, so taint analysis flagged the writes. The home directory is now validated as absolute before any path is derived from it, and the writes carry a justification, matching how this repository already handles the same pattern. G101: a test built a fake credential inline as a password-in-URL literal. Now assembled from parts so it is not read as a hardcoded secret. Also removes the design note that was committed earlier; design discussion does not belong in the branch.
…ative-support # Conflicts: # artifactory/commands/setup/setup.go # artifactory/commands/setup/setup_test.go # go.mod
Merging main brought in packageManagerConfigs, which drives the note printed after a successful setup and is asserted to cover every supported package manager. Ruby was registered as supported but had no entry, so the map held 17 of the expected 18 and the setup tests failed. Ruby writes ~/.gemrc and ~/.bundle/config directly, always under the user's home directory, and honours no override variable of its own, so it is described as a user-level configuration change rather than a credentials-only one.
The RubyGems FlexPack branch now contains main, so its pseudo-version is newer than main's. Consumers can therefore select it through an ordinary require instead of a replace directive, which previously could not pin backwards to a commit older than main.
reshmifrog
requested changes
Aug 6, 2026
| return fmt.Errorf("unsupported ruby tool %q: expected 'gem' or 'bundle'", rc.nativeTool) | ||
| } | ||
|
|
||
| // Bug 3 fix: explicit no-args check before help bypass so we don't |
Contributor
There was a problem hiding this comment.
remove this comment, wdym by bug 3 fix?
Collaborator
Author
There was a problem hiding this comment.
Fixed — reworded to describe what the check does instead of referencing an internal bug number with no context here.
| subCommand := rubySubCommand(rc.args) | ||
|
|
||
| // Help requests bypass auth injection entirely so credentials are never | ||
| // printed in help output (same rationale as the UV native command). |
Contributor
There was a problem hiding this comment.
this comment does not look good , refactor it and write exactly what this func is doing .
Collaborator
Author
There was a problem hiding this comment.
Reworded — it was pointing at "the UV native command" for the rationale instead of just stating it directly.
| } | ||
|
|
||
| log.Info(fmt.Sprintf("Running %s %s.", rc.nativeTool, subCommand)) | ||
| // For gem install/fetch, capture stdout to parse "Successfully installed"/"Downloaded" lines. |
Contributor
There was a problem hiding this comment.
let's not parse the stdout , because the format can change and it would break our code
Collaborator
Author
There was a problem hiding this comment.
Looked for a machine-readable alternative before replying — gem install --help has no JSON/structured output mode, so there isn't a clean way to know exactly what was installed/fetched by this invocation without either parsing stdout or diffing gem-list state before/after (which has its own races). The code already falls back to explicit args + gem list when stdout parsing yields nothing (see collectGemInstallDependencies). Open to a different approach if you have a specific one in mind — this was the least fragile option I found.
|
|
||
| // rubyEmbedCredsInSourceArg rewrites --source/--host URL args to embed credentials | ||
| // for gem install/fetch. RubyGems 3.x uses embedded URL credentials for index downloads | ||
| // (specs.4.8.gz) but does NOT use GEM_HOST_API_KEY for those requests. |
Contributor
There was a problem hiding this comment.
this comment is very confusing , not able to understand what exactly this function is doing.
Collaborator
Author
There was a problem hiding this comment.
Reworded to state directly why GEM_HOST_API_KEY doesn't cover this path (RubyGems 3.x reads the index via URL-embedded credentials for this specific request, not the env var).
|
|
||
| // Ensure ~/.gem directory exists. | ||
| if err := os.MkdirAll(gemDir, 0700); err != nil { | ||
| return nil, fmt.Errorf("could not create ~/.gem directory: %w", err) |
Contributor
There was a problem hiding this comment.
why are we creating this the setup command would create it right?
Collaborator
Author
There was a problem hiding this comment.
This is deliberate, not an oversight: jf ruby is a config-less flow (see the doc comment at the top of ruby.go) that does not assume jf setup ruby ran first, so gem push needs to be able to create ~/.gem itself on a machine where setup was never run.
|
|
||
| // Determine the host to authenticate. Prefer the discovered source URL host; | ||
| // otherwise fall back to the Artifactory server host. | ||
| host := rubyHostOf(sourceURL) |
Contributor
There was a problem hiding this comment.
it has to be always artifactory right? what do mean by the Prefer the discovered source URL host; otherwise fall back to the Artifactory server host.
Collaborator
Author
There was a problem hiding this comment.
The logic is correct as written: authorizedForSource gates every credential path on the target host matching the configured Artifactory host (or an explicit --server-id), so in practice this never authenticates against an arbitrary registry — only the discovered source or the server config host, whichever resolved. Happy to reword the comment further if the wording itself is still unclear — let me know what specifically read as ambiguous.
| return err | ||
| } | ||
|
|
||
| // For gem install/fetch: parse stdout to determine exactly what was installed/fetched. |
Contributor
There was a problem hiding this comment.
we should not parse stdout anywhere , please check it everywhere and try ot not use it.
Collaborator
Author
There was a problem hiding this comment.
Same answer as the other stdout-parsing comment on this file — checked and there's no structured output mode for gem install/fetch to switch to. Happy to discuss alternatives if you have one in mind.
| "Point your Gemfile/gem source at an Artifactory gems repository or pass --server-id.") | ||
| } | ||
|
|
||
| if err := rubySaveBuildInfo(bi, rc.buildConfiguration); err != nil { |
Contributor
There was a problem hiding this comment.
why we have rubySave BuildInfo? i think we already have a function named SaveBuidInfo why seperate for ruby?
Collaborator
Author
There was a problem hiding this comment.
Not a duplicate — rubySaveBuildInfo is a thin per-command wrapper around the shared bld.SaveBuildInfo(bi), the same pattern the UV command uses (see native_uv.go), not a reimplementation of it.
| // collectGemBuildDependencies records the dependencies a `gem build` was built against, | ||
| // read from Gemfile.lock. Without a lock file there is nothing resolved to record, and | ||
| // saying so is better than accepting --build-name and silently producing nothing. | ||
| func (rc *RubyCommand) collectGemBuildDependencies(workingDir, repoKey string, serverDetails *coreConfig.ServerDetails) error { |
Contributor
There was a problem hiding this comment.
i see this same function repeating , remove the duplicate method.
Collaborator
Author
There was a problem hiding this comment.
Removed rubyEmbedCredsInHostArg — it was dead code (zero callers) and nearly identical to rubyEmbedCredsInSourceArg. If this is the duplication you meant, it's gone now. collectGemBuildDependencies itself isn't a duplicate of anything — it delegates to the shared collectLockfileDependencies, the same helper the bundle-install path uses.
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.
Summary
Implements the core business logic for the
jf ruby gem|bundlenative command in jfrog-cli-artifactory, covering authentication injection, repository discovery, native tool execution, and build-info collection.Depends on: jfrog/build-info-go#394
What is included in this PR
gemorbundlewith full stdio passthroughBUNDLE_<HOST>with full host-key normalization (. -> __, - -> ___, etc.)GEM_HOST_API_KEYwith properBasic base64(user:pass)encodinggem install/fetch --source(for index downloads)--repoflag: Constructs the full Artifactory gems API URL from server config + repo name, eliminating the need to pass full URLs. Auto-injects--source/--hostfor gem install/push.bundle install/update/lock/addand opportunisticgem install/fetchgem build(local .gem checksums) andgem push(+ build property tagging in Artifactory)Bug fixes in this update
GEM_HOST_API_KEYnow encodes asBasic base64(user:pass)— Artifactory expects this formatgem install/fetchembeds credentials directly in--sourceURL for index downloads (specs.4.8.gz) sinceGEM_HOST_API_KEYis not used for those requests in RubyGems 3.xgem help--server-id)Auth level
Server-level (host-level) — one credential covers all repos on the same Artifactory instance. Not repo-level.
What will be in follow-up PRs
jf setupintegration for Ruby, paginated AQL, cached build-info reuse, multi-source Gemfile authTest plan
go test ./artifactory/commands/ruby/... -v- all tests passgo build ./...- compiles successfully (with local build-info-go replace)go vet ./artifactory/commands/ruby/...- no issuesSummary by CodeRabbit
New Features
jf setup rubyto configure Bundler credentials and RubyGems sources automatically.Bug Fixes
Tests