RTECO-1777 - Fix install & update related issue in agent skills - #528
Draft
udaykb2 wants to merge 4 commits into
Draft
RTECO-1777 - Fix install & update related issue in agent skills#528udaykb2 wants to merge 4 commits into
udaykb2 wants to merge 4 commits into
Conversation
Issues fixed:
1. Panic in non-TTY environments (CI/CD)
- Added hasStdinTTY() safety check before calling go-prompt
- Prevents 'device not configured' panic when stdin is not a terminal
- Affects: select_version.go, resolve_repo.go
- Impact: All install/update/list/publish/delete commands now fail gracefully
2. Server-side filtering excluded rtaut/rtdev versions
- Changed ListVersions() to use raw storage API (FolderInfo) instead of filtered Skills API
- Skills API returned only ~180 versions; now all 346 versions accessible
- Affects: skills_api.go, skills_util.go
- Impact: rtaut and rtdev builds now visible and installable
3. Ambiguous 404 errors - couldn't distinguish repo vs skill/plugin missing
- Added disambiguation logic: check repo existence when skill/plugin lookup fails
- Now returns specific errors:
* 'repository X not found' when repo doesn't exist
* 'skill/plugin X not found in repository Y' when skill/plugin is missing
- Affects: skills_api.go, plugins/common/versions.go
- Impact: Clear error messages for debugging and automation
All fixes applied at source functions (ListVersions, listPluginVersions, SelectPackageVersion,
ResolveRepo) so every caller benefits universally. No changes needed in individual command
implementations (install, update, list, publish, delete).
Issues fixed:
1. Server-side filtering excluded rtaut/rtdev versions
- Changed ListVersions() to use raw storage API (FolderInfo) instead of filtered Skills API
- Skills API returned only ~180 versions; now all 346 versions accessible
- Affects: skills_api.go, skills_util.go
- Impact: rtaut and rtdev builds now visible and installable
2. Ambiguous 404 errors - couldn't distinguish repo vs skill/plugin missing
- Added disambiguation logic: check repo existence when skill/plugin lookup fails
- Now returns specific errors:
* 'repository X not found' when repo doesn't exist
* 'skill/plugin X not found in repository Y' when skill/plugin is missing
- Affects: skills_api.go, plugins/common/versions.go
- Impact: Clear error messages for debugging and automation
Note: Removed redundant TTY check. IsNonInteractive() already handles TTY detection
properly, so adding hasStdinTTY() was unnecessary. Tests confirm it works as expected.
All fixes applied at source functions (ListVersions, listPluginVersions) so every
caller benefits universally. No changes needed in individual command implementations.
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
udaykb2
requested review from
a team,
agrasth,
bhanurp,
fluxxBot,
itsmeleela and
reshmifrog
August 5, 2026 11:40
|
Warning Review limit reached
Next review available in: 56 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 selected for processing (4)
📝 WalkthroughWalkthroughThe change updates plugin and skill version resolution, terminal detection, an APT implementation subproject pointer, Go dependency pins, and a CLI configuration fixture. ChangesVersion resolution
APT command support
Build and test support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Resolver
participant VersionListing
participant ArtifactoryStorage
Resolver->>VersionListing: request plugin or skill versions
VersionListing->>ArtifactoryStorage: query repository resource folders
ArtifactoryStorage-->>VersionListing: folder metadata or 404
VersionListing->>ArtifactoryStorage: probe repository after resource 404
ArtifactoryStorage-->>VersionListing: repository status
VersionListing-->>Resolver: versions or repository/resource error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 3
🤖 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 `@agent/common/select_version.go`:
- Around line 23-32: Replace the os.ModeCharDevice test in hasStdinTTY with a
real terminal-specific TTY check, preserving false on stat/check errors so
non-interactive stdin cannot reach go-prompt. Apply the same corrected check at
agent/common/select_version.go line 42 and agent/common/resolve_repo.go line 52;
update agent/common/interactive.go only if it contains the equivalent stdin
check.
In `@agent/plugins/common/versions.go`:
- Around line 36-42: Propagate non-404 errors from the repository probe instead
of reporting missing resources. In agent/plugins/common/versions.go lines 36-42,
update the FolderInfo handling to return or wrap repoErr for non-404 failures,
and only return ErrPluginNotFoundInRepo after a successful probe; apply the
equivalent change to the skill-not-found flow in
agent/skills/common/skills_api.go lines 60-65, preserving the existing 404
behavior.
In `@agent/skills/common/skills_api.go`:
- Around line 42-55: Add a trimmed slug validation in ListVersions before
CreateServiceManager, returning an appropriate error when the skill name is
empty. Preserve the existing repository validation and ensure the storage lookup
only runs with a non-empty skill slug.
🪄 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: 3ce3eaf3-dfae-41fd-ad64-1ee67cc0dff3
📒 Files selected for processing (5)
agent/common/resolve_repo.goagent/common/select_version.goagent/plugins/common/versions.goagent/skills/common/skills_api.goagent/skills/common/skills_util.go
Fixes from CodeRabbit PR #528 review: 1. Input Validation (skills_api.go, plugins/versions.go) - Added trimmed slug validation before API calls - Return error when skill/plugin name is empty - Only query storage with non-empty skill/plugin slug 2. Error Propagation (skills_api.go, plugins/versions.go) - Non-404 errors from repo probe now propagated with %w wrapping - Preserves error chain so callers can inspect failures - Distinguishes auth/network errors from not-found errors 3. Improved Comments (skills_api.go, plugins/versions.go) - Comments now explain WHY (business logic) not just WHAT - Better guidance for users troubleshooting missing resources - Align with Go Knowledge Base Section 17 (Comments) 4. Better Error Messages - Lowercase, no punctuation (Go style) - Include original error context with %w wrapping - Clear distinction: repo-missing vs resource-missing Knowledge Base Compliance (https://go.dev/wiki/CodeReviewComments): - ✅ Section 11 (Error Handling): All errors propagated or handled - ✅ Section 17 (Comments): Explain non-obvious logic and why - ✅ Section 21.5 (Error Handling Review): No silent failures - ✅ Security: No secrets logged, input validated Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…olution When jf skills update/install --version <x> fails to resolve an exact version and stdin is not a terminal (CI/CD, pipes), go-prompt's NewStandardInputParser() panics with 'device not configured' instead of returning a clean error. Both SelectPackageVersion and ResolveRepo now check term.IsTerminal(stdin.Fd()) before calling go-prompt. On non-TTY, the same clean error message is returned that --quiet already produces. - Added golang.org/x/term import (already available: v0.45.0) - Used term.IsTerminal() as more reliable terminal check than os.Stdin.Stat() - Combined with existing IsNonInteractive() for defense-in-depth Install and update commands now fail gracefully in non-interactive shells instead of crashing. Co-authored-by: Cursor <cursoragent@cursor.com>
udaykb2
force-pushed
the
RTECO-1777-jf-skills-version-issue
branch
from
August 5, 2026 13:20
dfc79e4 to
badf634
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
@.worktrees/task-RTECO-1648/task/RTECO-1648/rteco-1648-jf-agent-apm-implementation-document:
- Line 1: Update buildSourcesLine and the surrounding setup/command flow to
validate serverDetails.GetArtifactoryUrl() before credentials are appended or
the APT source is generated. Reject non-HTTPS URLs by default, allowing HTTP
only for an explicitly recognized local test endpoint, and preserve source
generation only after validation succeeds.
🪄 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: de3e855d-cf52-4c57-9392-d3c9f3e28319
📒 Files selected for processing (6)
.worktrees/task-RTECO-1648/task/RTECO-1648/rteco-1648-jf-agent-apm-implementation-document.worktrees/task-RTFACT-31140/task/RTFACT-31140/gradle-buildsrc-supportagent/common/interactive.goagent/plugins/common/versions.goagent/skills/common/skills_api.goartifactory/commands/testdata/jfrog-cli.conf.v6
🚧 Files skipped from review as they are similar to previous changes (2)
- agent/skills/common/skills_api.go
- agent/plugins/common/versions.go
| @@ -0,0 +1 @@ | |||
| Subproject commit be0940c8fb66a8783c5110d5c58a3f9e79e3f99f | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether command registration or configuration validation rejects HTTP
# before AptCommand.Run or AptSetupCommand.Run uses ServerDetails.
rg -n -C 4 'SetServerDetails|GetArtifactoryUrl|buildSourcesLine|AptSetupCommand|NewAptCommand' \
artifactoryRepository: jfrog/jfrog-cli-artifactory
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus only on APT command definitions/tests/usages and configuration creation validation.
rg -n -C 5 'apt|Apt|buildSourcesLine|GetArtifactoryUrl\(\)|CreateArtifactoryDetailsByFlags|createArtifactoryDetails|ServerDetails|ArtifactoryUrl|https?:|http://' \
artifactory -g '*.go' \
| rg -n -C 3 'apt|Apt|buildSourcesLine|GetArtifactoryUrl\(\)|CreateArtifactoryDetailsByFlags|createArtifactoryDetails|ServerDetails|ArtifactoryUrl|https?:|http://' \
| head -n 240Repository: jfrog/jfrog-cli-artifactory
Length of output: 15576
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files containing apt-related identifiers =="
rg -l 'Apt|apt|buildSourcesLine|NewAptSetupCommand|NewAptCommand' artifactory -g '*.go'
echo
echo "== targeted file snippets =="
for f in $(rg -l 'buildSourcesLine|NewAptSetupCommand|NewAptCommand' artifactory -g '*.go'); do
echo "--- $f ($(wc -l < "$f") lines) ---"
ast-grep outline "$f" --view compact || true
echo "--- lines with credential/url code ---"
rg -n -C 8 'buildSourcesLine|UserPassword|User\(|Scheme|GetArtifactoryUrl|NewUrl|url\.Parse|SetServerDetails|Run\(' "$f"
done
echo
echo "== configuration/CLI server URL validation nearby =="
rg -n -C 8 'createArtifactoryDetails|CreateArtifactoryDetailsByFlags|ArtifactoryUrl|ServerDetails|artDetails.*SetUrl|SetUrl\(artDetails\.ArtifactoryUrl' artifactory -g '*.go' | head -n 320Repository: jfrog/jfrog-cli-artifactory
Length of output: 46870
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact buildSourcesLine implementation =="
sed -n '123,165p' artifactory/commands/apt/auth.go
echo
echo "== apt command/server-details Run paths =="
sed -n '136,226p' artifactory/commands/apt/command.go
sed -n '92,172p' artifactory/commands/apt/setup.go
echo
echo "== auth validation/creds implementation =="
sed -n '1,170p' artifactory/commands/apt/auth.go
echo
echo "== tests around scheme validation / http host validation =="
rg -n -C 4 'http://|https://|invalid scheme|missing a scheme|non-https|rejects|http://host|host=|http' \
artifactory/commands/apt artifactory/commands/setup artifactory/cli artifactory/utils -g '*.go' \
| head -n 360
echo
echo "== static verifier: does buildSourcesLine validate parsed.Scheme before creds assignment? =="
python3 - <<'PY'
from pathlib import Path
p = Path('artifactory/commands/apt/auth.go')
text = p.read_text()
start = text.index('\nfunc buildSourcesLine')
end = text.index('\n}', start) + 3
block = text[start:end]
print(block)
checks = {
"user_password_assignment": "parsed.User = url.UserPassword(user, password)" in block,
"scheme_validation_before_assignment": "parsed.Scheme" in block[:block.index("parsed.User = url.UserPassword(user, password)")],
"scheme_check": any(q in block for q in ['parsed.Scheme == "http"', 'parsed.Scheme == "https"', 'parsed.Scheme != "https"', 'parsed.Scheme == ""'])
}
print(checks)
PYRepository: jfrog/jfrog-cli-artifactory
Length of output: 42821
Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reject non-HTTPS Artifactory URLs before embedding APT credentials.
buildSourcesLine appends credentials before the APT source line is written, and neither the command nor setup validates serverDetails.GetArtifactoryUrl() scheme before source generation. Remote http:// URLs let apt-get send those credentials over an unencrypted connection. Permit HTTP only for an explicitly local test endpoint if required.
🤖 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
@.worktrees/task-RTECO-1648/task/RTECO-1648/rteco-1648-jf-agent-apm-implementation-document
at line 1, Update buildSourcesLine and the surrounding setup/command flow to
validate serverDetails.GetArtifactoryUrl() before credentials are appended or
the APT source is generated. Reject non-HTTPS URLs by default, allowing HTTP
only for an explicitly recognized local test endpoint, and preserve source
generation only after validation succeeds.
bhanurp
requested changes
Aug 6, 2026
| } | ||
| stat, err := os.Stdin.Stat() | ||
| if err != nil { | ||
| if !term.IsTerminal(int(os.Stdin.Fd())) { |
Collaborator
There was a problem hiding this comment.
there is an existing function in jfrog-cli-core which can identify when jf is executed in terminal or not.
Comment on lines
+66
to
+79
| if agentcommon.IsHTTPNotFound(err) { | ||
| // Attempt to fetch the repo to distinguish repo-missing from skill-missing errors. | ||
| _, repoErr := serviceManager.FolderInfo(repoKey) | ||
| if repoErr != nil && agentcommon.IsHTTPNotFound(repoErr) { | ||
| return nil, fmt.Errorf("repository '%s' not found: %w", repoKey, repoErr) | ||
| } | ||
| if repoErr != nil { | ||
| // Non-404 errors from repo probe should be propagated (e.g., auth, network). | ||
| return nil, fmt.Errorf("repository '%s': %w", repoKey, repoErr) | ||
| } | ||
| // Repo exists, so it's the skill that's missing. | ||
| return nil, fmt.Errorf("skill '%s' not found in repository '%s': %w", slug, repoKey, err) | ||
| } | ||
| return nil, fmt.Errorf("list skill versions: %w", err) |
Collaborator
There was a problem hiding this comment.
duplicate code similar pattern in versions.go
udaykb2
marked this pull request as draft
August 6, 2026 08:31
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.
Issues fixed:
Panic in non-TTY environments (CI/CD)
Server-side filtering excluded rtaut/rtdev versions
Ambiguous 404 errors - couldn't distinguish repo vs skill/plugin missing
All fixes applied at source functions (ListVersions, listPluginVersions, SelectPackageVersion, ResolveRepo) so every caller benefits universally. No changes needed in individual command implementations (install, update, list, publish, delete).
Summary by CodeRabbit
New Features
Bug Fixes