Skip to content

Making the name resolution part in the system prompt more strict.#259

Merged
openshift-merge-bot[bot] merged 1 commit intorh-ecosystem-edge:mainfrom
andrej1991:stable_uuid_resolution
Nov 21, 2025
Merged

Making the name resolution part in the system prompt more strict.#259
openshift-merge-bot[bot] merged 1 commit intorh-ecosystem-edge:mainfrom
andrej1991:stable_uuid_resolution

Conversation

@andrej1991
Copy link
Copy Markdown
Contributor

@andrej1991 andrej1991 commented Nov 21, 2025

Also with the current changes the name resolution part covers more cases. Previously they were thought to be "common sense". The background of the issue is that the evaluation test cluster_info_conv/cluster_info_tool_call failed sometimes either because it thought that the name is the uuid, or it did not echoed the name of the cluster when it was not able to find it.
I think echoing the name of the cluster back when not able to find it helps the user identify typos.

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Improved cluster name resolution: when using non-UUID cluster identifiers, the system now automatically scans all known clusters for exact matches.
    • Enhanced error messaging that explicitly states when a provided cluster name does not exist.

✏️ Tip: You can customize this high-level summary in your review settings.

Also with the current changes the name resolution part covers more cases. Previously they were thought to be "common sense".
The background of the issue is that the evaluation test
cluster_info_conv/cluster_info_tool_call failed sometimes either because
it thought that the name is the uuid, or it did not echoed the name of
the cluster when it was not able to find it.
I think echoing the name of the cluster back when not able to find it
helps the user identify typos.
@openshift-ci openshift-ci bot requested review from carbonin and eranco74 November 21, 2025 21:06
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Nov 21, 2025

Walkthrough

The template modifies the cluster name resolution logic to first list all known clusters and perform exact-name matching before requesting a Cluster ID. Additionally, the apiVersion declaration is removed from the llama-stack ConfigMap resource declaration.

Changes

Cohort / File(s) Change Summary
Cluster name resolution logic
template.yaml
Modified cluster name-to-ID mapping flow to include cluster listing step for non-UUID names; updated matching logic to require user clarification on multiple matches and explicitly state non-existence when no matches found.
ConfigMap resource declaration
template.yaml
Removed apiVersion line from llama-stack-client-config ConfigMap resource definition.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant System
    participant ClusterAPI

    User->>System: Provide cluster name (non-UUID)
    
    rect rgb(200, 220, 255)
    Note over System,ClusterAPI: New: List all clusters
    System->>ClusterAPI: List all known clusters
    ClusterAPI-->>System: Cluster list with names & IDs
    end
    
    System->>System: Perform exact-name match
    
    alt Exactly one match found
        System-->>User: Map name to Cluster ID
        User->>System: Proceed with operation
    else Multiple matches found
        System-->>User: Request clarification on which Cluster ID
        User->>System: Provide Cluster ID
    else No matches found
        rect rgb(255, 220, 200)
        Note over System,User: New: Explicit messaging
        System-->>User: Cluster name does not exist
        end
        System-->>User: Prompt for Cluster ID
        User->>System: Provide Cluster ID
    end
    
    System->>ClusterAPI: Perform operation with Cluster ID
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Review cluster name resolution logic for correctness of exact-match search implementation
  • Verify user messaging changes accurately reflect the new behavior
  • Confirm ConfigMap apiVersion removal does not impact manifest deployment or validation

Possibly related PRs

Suggested labels

approved, lgtm

Suggested reviewers

  • jhernand
  • keitwb
  • carbonin

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title mentions making name resolution 'more strict', but the actual changes involve clarifying error messages, handling non-UUID inputs differently, and adding steps to list clusters before searching. The title is vague about the specific improvements and doesn't clearly convey the main behavioral changes. Consider a more descriptive title that captures the core changes, such as: 'Improve cluster name resolution with explicit error messaging and cluster listing step' or 'Enhance cluster name resolution logic with better error handling and clarification steps'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fa750d2 and b18d365.

📒 Files selected for processing (1)
  • template.yaml (1 hunks)
🔇 Additional comments (2)
template.yaml (2)

335-340: Strengthened cluster name resolution logic is well-structured.

The new strict logic for cluster name resolution is clear and actionable. It correctly implements the PR objective: when a cluster name cannot be resolved, the system will explicitly state the name doesn't exist (echoing it back implicitly within the "doesn't exist" statement), helping users identify typos. The progression from silent listing → exact-match search → three outcome branches aligns well with the rest of the system prompt's guidance style.


346-346: Inconsistency: AI summary claims apiVersion removed, but code shows it present.

The AI summary states that the "apiVersion declaration is removed from the first llama-stack ConfigMap stanza," but line 346 shows - apiVersion: v1 present and unmarked (indicating no change in this diff). This contradicts the summary claim. Clarify whether the apiVersion removal was intentional but not reflected in the provided code, or whether the summary is inaccurate.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Nov 21, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrej1991, carbonin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit e0b6086 into rh-ecosystem-edge:main Nov 21, 2025
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants